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

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.

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

--
Alex