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

On 11/25/2014 01:37 PM, Alex Shulgin wrote:
>
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)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