Re: Missing OOM checks in libpq (was Re: Replication connection URI?)

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

> On 11/24/2014 06:05 PM, Alex Shulgin wrote:
>> The first patch is not on topic, I just spotted this missing check.
>
>> *** a/src/interfaces/libpq/fe-connect.c
>> --- b/src/interfaces/libpq/fe-connect.c
>> *************** conninfo_array_parse(const char *const *
>> *** 4402,4407 ****
>> --- 4402,4415 ----
>>                                   if (options[k].val)
>>                                       free(options[k].val);
>>                                   options[k].val = strdup(str_option->val);
>> +                                 if (!options[k].val)
>> +                                 {
>> +                                     printfPQExpBuffer(errorMessage,
>> +                                                       libpq_gettext("out of memory\n"));
>> +                                     PQconninfoFree(options);
>> +                                     PQconninfoFree(dbname_options);
>> +                                     return NULL;
>> +                                 }
>>                                   break;
>>                               }
>>                           }
>
> Oh. There are actually many more places in connection option parsing
> that don't check the return value of strdup(). The one in fillPGConn
> even has an XXX comment saying it probably should check it. You can
> get quite strange behavior if one of them fails. If for example the
> strdup() on dbname fails, you might end up connecting to different
> database than intended. And if the "conn->sslmode =
> strdup(DefaultSSLMode);" call in connectOptions2 fails, you'll get a
> segfault later because at least connectDBstart assumes that sslmode is
> not NULL.
>
> I think we need to fix all of those, and backpatch. Per attached.

Yikes!  Looks sane to me.

I've checked that patches #2 and #3 can be applied on top of this, w/o
rebasing.

--
Alex