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

Hackers,

It appears that replication connection doesn't support URI but only the
traditional conninfo string.

src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():

snprintf(conninfo_repl, sizeof(conninfo_repl),
"%s dbname=replication replication=true fallback_application_name=walreceiver",
conninfo);

A patch to fix this welcome?

--
Alex

PS: I wrote the original URI parser used in libpq.

On 11/24/2014 02:41 PM, Alex Shulgin wrote:
>
> Hackers,
>
> It appears that replication connection doesn't support URI but only the
> traditional conninfo string.
>
> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>
> snprintf(conninfo_repl, sizeof(conninfo_repl),
> "%s dbname=replication replication=true fallback_application_name=walreceiver",
> conninfo);
>
> A patch to fix this welcome?

Yeah, seems like an oversight. Hopefully you can fix that without
teaching libpqwalreceiver what connection URIs look like..

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>
>> It appears that replication connection doesn't support URI but only the
>> traditional conninfo string.
>>
>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>>
>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>> "%s dbname=replication replication=true fallback_application_name=walreceiver",
>> conninfo);
>>
>> A patch to fix this welcome?
>
> Yeah, seems like an oversight. Hopefully you can fix that without
> teaching libpqwalreceiver what connection URIs look like..

Please see attached. We're lucky that PQconnectdbParams has an option
to parse and expand the first dbname parameter if it looks like a
connection string (or a URI).

The first patch is not on topic, I just spotted this missing check.

The second one is a self-contained fix, but the third one which is the
actual patch depends on the second one, because it specifies the dbname
keyword two times: first to parse the conninfo/URI, then to override any
dbname provided by the user with "replication" pseudo-database name.

Have a nice day!
--
Alex

Alex Shulgin <ash(at)commandprompt(dot)com> writes:

> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>
>>> It appears that replication connection doesn't support URI but only the
>>> traditional conninfo string.
>>>
>>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>>>
>>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>>> "%s dbname=replication replication=true fallback_application_name=walreceiver",
>>> conninfo);
>>>
>>> A patch to fix this welcome?
>>
>> Yeah, seems like an oversight. Hopefully you can fix that without
>> teaching libpqwalreceiver what connection URIs look like..
>
> Please see attached. We're lucky that PQconnectdbParams has an option
> to parse and expand the first dbname parameter if it looks like a
> connection string (or a URI).
>
> The first patch is not on topic, I just spotted this missing check.
>
> The second one is a self-contained fix, but the third one which is the
> actual patch depends on the second one, because it specifies the dbname
> keyword two times: first to parse the conninfo/URI, then to override any
> dbname provided by the user with "replication" pseudo-database name.

These patches are really simple, I hope a committer will pick them up?
Or should I add them to the commitfest?

Also, I'd rather get this committed first, then rebase that
recovery.conf->GUC patch onto it and submit an updated version.

Thanks.
--
Alex

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.

- Heikki

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

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

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>
>>> 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.

Great. Are you looking at the actual replication URI patch? Or should
I add it to commitfest so we don't lose track of it?

--
Alex

On 11/24/2014 06:05 PM, Alex Shulgin wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>
>>> It appears that replication connection doesn't support URI but only the
>>> traditional conninfo string.
>>>
>>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>>>
>>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>>> "%s dbname=replication replication=true fallback_application_name=walreceiver",
>>> conninfo);
>>>
>>> A patch to fix this welcome?
>>
>> Yeah, seems like an oversight. Hopefully you can fix that without
>> teaching libpqwalreceiver what connection URIs look like..
>
> Please see attached. We're lucky that PQconnectdbParams has an option
> to parse and expand the first dbname parameter if it looks like a
> connection string (or a URI).
>
> The first patch is not on topic, I just spotted this missing check.
>
> The second one is a self-contained fix, but the third one which is the
> actual patch depends on the second one, because it specifies the dbname
> keyword two times: first to parse the conninfo/URI, then to override any
> dbname provided by the user with "replication" pseudo-database name.

Hmm. Should we backpatch the second patch? It sure seems like an
oversight rather than deliberate that you can't override dbname from the
connection string with a later dbname keyword. I'd say "yes".

How about the third patch? Probably not; it was an oversight with the
connection URI patch that it could not be used in primary_conninfo, but
it's not a big deal in practice as you can always use a non-URI
connection string instead.

- Heikki

On 11/25/2014 05:11 PM, Heikki Linnakangas wrote:
> On 11/24/2014 06:05 PM, Alex Shulgin wrote:
>> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>>
>>>> It appears that replication connection doesn't support URI but only the
>>>> traditional conninfo string.
>>>>
>>>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>>>>
>>>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>>>> "%s dbname=replication replication=true fallback_application_name=walreceiver",
>>>> conninfo);
>>>>
>>>> A patch to fix this welcome?
>>>
>>> Yeah, seems like an oversight. Hopefully you can fix that without
>>> teaching libpqwalreceiver what connection URIs look like..
>>
>> Please see attached. We're lucky that PQconnectdbParams has an option
>> to parse and expand the first dbname parameter if it looks like a
>> connection string (or a URI).
>>
>> The first patch is not on topic, I just spotted this missing check.
>>
>> The second one is a self-contained fix, but the third one which is the
>> actual patch depends on the second one, because it specifies the dbname
>> keyword two times: first to parse the conninfo/URI, then to override any
>> dbname provided by the user with "replication" pseudo-database name.
>
> Hmm. Should we backpatch the second patch? It sure seems like an
> oversight rather than deliberate that you can't override dbname from the
> connection string with a later dbname keyword. I'd say "yes".
>
> How about the third patch? Probably not; it was an oversight with the
> connection URI patch that it could not be used in primary_conninfo, but
> it's not a big deal in practice as you can always use a non-URI
> connection string instead.

Ok, committed the second patch to all stable branches, and the third
patch to master.

In the second patch, I added a sentence to the docs to mention that only
the first "dbname" parameter is expanded. It's debatable if that's what
we actually want. It would be handy to be able to merge multiple
connection strings, by specifying multiple dbname parameters. But now
the docs at least match the code, changing the behavior would be a
bigger change.

From the third patch, I removed the docs changes. It's necessary to say
"connection string or URI" everywhere, the URI format is just one kind
of a connection string. I also edited the code that builds the
keyword/value array, to make it a bit more readable.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>
>>> The second one is a self-contained fix, but the third one which is the
>>> actual patch depends on the second one, because it specifies the dbname
>>> keyword two times: first to parse the conninfo/URI, then to override any
>>> dbname provided by the user with "replication" pseudo-database name.
>>
>> Hmm. Should we backpatch the second patch? It sure seems like an
>> oversight rather than deliberate that you can't override dbname from the
>> connection string with a later dbname keyword. I'd say "yes".
>>
>> How about the third patch? Probably not; it was an oversight with the
>> connection URI patch that it could not be used in primary_conninfo, but
>> it's not a big deal in practice as you can always use a non-URI
>> connection string instead.
>
> Ok, committed the second patch to all stable branches, and the third
> patch to master.
>
> In the second patch, I added a sentence to the docs to mention that
> only the first "dbname" parameter is expanded. It's debatable if
> that's what we actually want. It would be handy to be able to merge
> multiple connection strings, by specifying multiple dbname
> parameters. But now the docs at least match the code, changing the
> behavior would be a bigger change.
>
> From the third patch, I removed the docs changes. It's necessary to
> say "connection string or URI" everywhere, the URI format is just one
> kind of a connection string. I also edited the code that builds the
> keyword/value array, to make it a bit more readable.

Yay, many thanks! :-)

--
Alex

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:

> On 11/25/2014 05:11 PM, Heikki Linnakangas wrote:
>> On 11/24/2014 06:05 PM, Alex Shulgin wrote:
>>> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>>>>>
>>>>> It appears that replication connection doesn't support URI but only the
>>>>> traditional conninfo string.
>>>>>
>>>>> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in libpqrcv_connect():
>>>>>
>>>>> snprintf(conninfo_repl, sizeof(conninfo_repl),
>>>>> "%s dbname=replication replication=true fallback_application_name=walreceiver",
>>>>> conninfo);
>>>>>
>>>>> A patch to fix this welcome?
>>>>
>>>> Yeah, seems like an oversight. Hopefully you can fix that without
>>>> teaching libpqwalreceiver what connection URIs look like..
>>>
>>> Please see attached. We're lucky that PQconnectdbParams has an option
>>> to parse and expand the first dbname parameter if it looks like a
>>> connection string (or a URI).
>>>
>>> The first patch is not on topic, I just spotted this missing check.
>>>
>>> The second one is a self-contained fix, but the third one which is the
>>> actual patch depends on the second one, because it specifies the dbname
>>> keyword two times: first to parse the conninfo/URI, then to override any
>>> dbname provided by the user with "replication" pseudo-database name.
>>
>> Hmm. Should we backpatch the second patch? It sure seems like an
>> oversight rather than deliberate that you can't override dbname from the
>> connection string with a later dbname keyword. I'd say "yes".
>>
>> How about the third patch? Probably not; it was an oversight with the
>> connection URI patch that it could not be used in primary_conninfo, but
>> it's not a big deal in practice as you can always use a non-URI
>> connection string instead.
>
> Ok, committed the second patch to all stable branches, and the third
> patch to master.

It still looks like a bug that primary_conninfo doesn't accept URIs,
even though they were supposed to be handled transparently by all
interfaces using libpq...

Any chance we reconsider and back-patch this up to 9.2?

--
Alex