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

On 11/25/2014 01:37 PM, Alex Shulgin wrote:
>
> 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.

Ok thanks, committed. It didn't apply cleanly to 9.0, 9.1 and 9.2, so 
the patch for those branches looks a bit different.

- Heikki