Re: Another review of URI for libpq, v7 submission

I reviewed this and so far have not found any serious problems,
although as is par for the course it contains some of the fiddly bits
involved in any string manipulations in C. I made a few edits -- none
strictly necessary for correctness -- that the original author is free
audit and/or include[0]. I did put in some defensive programming
choices (instead of if/else if/elseif/else raise an error, even if the
latter is allegedly impossible) that I think are a good idea.

Loops around pointer increments are very fastidiously checked for
NUL-byteness, and those that aren't are carefully guarded by
invariants that seem like they should prevent an overrun. The nature
of the beast, I suppose, short of giving libpq a "StringData" like
struct and a small lexer to make it more clear that a subtle overrun
is not creeping in.

One thing I found puzzling was that in the latest revision the tests
appeared to be broken for me: all "@" signs were translated to "(at)".
Is that mangling applied by the archives, or something?

The test suite neatly tries to copy pg_regress's general "make
installcheck" style, but it likes to use my username as the database
rather than the standard "regression" as seen by pg_regress. It is
nice that a place to test connection strings and such is there, where
there was none before.

I am happy with the range and style of accepted URIs, and I think this
can stem the bleeding of the fragmentation already taking place at
large.

[0]: https://github.com/fdr/postgres/tree/uri, commit
e50ef375b7a731ca79bf5d3ca8b0bd69c97a9e71, aka the 'uri' branch

--
fdr

Excerpts from Daniel Farina's message of jue mar 15 05:49:50 -0300 2012:

> One thing I found puzzling was that in the latest revision the tests
> appeared to be broken for me: all "@" signs were translated to "(at)".
> Is that mangling applied by the archives, or something?

Ugh, ouch. Yeah, that was done by the archives. It seems that when
attachments are text/plain Mhonarc applies anti-spamming to them :-(
The original message doesn't have that problem. Sorry about that.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Daniel Farina <daniel(at)heroku(dot)com> writes:

> I reviewed this and so far have not found any serious problems,
> although as is par for the course it contains some of the fiddly bits
> involved in any string manipulations in C. I made a few edits -- none
> strictly necessary for correctness -- that the original author is free
> audit and/or include[0].

Thank you for the review, Daniel!

Apparently, I was on drugs when I've submitted v7, as it still contains
the bug for which to fix I was forced to move parts of the code back
into the main parser routine...

> I did put in some defensive programming choices (instead of if/else
> if/elseif/else raise an error, even if the latter is allegedly
> impossible) that I think are a good idea.

Yes, this is a good idea, I'll incorporate them in the patch. However,
this one doesn't work:
https://github.com/fdr/postgres/commit/4fad90fb243d9266b1003cfbcf8397f67269fad3

Neither '@' or '/' are mandatory in the URI anywhere after "scheme://",
so we can't just say it "should never happen." Running the regression
test with this commit applied highlights the problem.

> One thing I found puzzling was that in the latest revision the tests
> appeared to be broken for me: all "@" signs were translated to "(at)".
> Is that mangling applied by the archives, or something?

This is definitely mangling by the lists. I can see this has happened
to people before, e.g.:
http://archives.postgresql.org/pgsql-hackers/2010-01/msg00938.php

But that discussion didn't seem to lead to a solution for the problem.

I wonder if there's any evidence as to that mangling the email addresses
helps to reduce spam at all? I mean replacing "(at)" back to "@" and
"(dot)" to "." is piece of cake for a spam crawler.

What I see though, is that most of the message-id URLs are broken by the
mangling. Also I don't think everyone should just tolerate that it
messes with the attachments. Should we all suddenly use
application/octet-stream or application/gzip instead of text/plain?

> The test suite neatly tries to copy pg_regress's general "make
> installcheck" style, but it likes to use my username as the database
> rather than the standard "regression" as seen by pg_regress. It is
> nice that a place to test connection strings and such is there, where
> there was none before.

Oh, I don't see why we can't use "regression" too.

While testing this I've also noticed that there was some funny behavior
when you left out the final slash after hostname, e.g.:
"postgres://localhost/" vs. "postgres://localhost". It turned out in
the former case we were setting dbname conninfo parameter to an empty
string and in the latter one we didn't set it at all. The difference is
that when we do set it, the default dbname is derived from username, but
when we don't--$PGDATABASE is used. I've fixed this, so now both URIs
work the same way (both do not set dbname.)

It also appeared to me that with the current code, a wide range of
funny-looking URIs are considered valid, e.g.:

postgres://user@
postgres://@host
postgres://host:/

and, taking approach to the extreme:

postgres://:@:

This specifies empty user, password, host and port, and looks really
funny. I've added (actually revived) some checks to forbid setting
empty URI parts where it doesn't make much sense.

Finally, attached is v8. Hopefully I didn't mess things up too much.

--
Regards,
Alex

On Thu, Mar 15, 2012 at 7:36 AM, Alex Shulgin <ash(at)commandprompt(dot)com> wrote:
> I wonder if there's any evidence as to that mangling the email addresses
> helps to reduce spam at all?  I mean replacing "(at)" back to "@" and
> "(dot)" to "." is piece of cake for a spam crawler.

I suspect we're long past the point in Internet history where such
simple obfuscation could possibly matter.

> Finally, attached is v8.  Hopefully I didn't mess things up too much.

I'll give it another look-over. Do you have these in git somewhere? It
will help me save time on some of the incremental changes.

--
fdr

Daniel Farina <daniel(at)heroku(dot)com> writes:
>
>> Finally, attached is v8.  Hopefully I didn't mess things up too much.
>
> I'll give it another look-over. Do you have these in git somewhere? It
> will help me save time on some of the incremental changes.

Yes, I've just pushed my dev branch to this fork of mine:
https://github.com/a1exsh/postgres/commits/uri

On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
> https://github.com/a1exsh/postgres/commits/uri

The point of the patch is to have one string with all connection options,
in standard format, yes? So why does not this work:

db = PQconnectdb("postgres://localhost");

?

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:

> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>> https://github.com/a1exsh/postgres/commits/uri
>
> The point of the patch is to have one string with all connection options,
> in standard format, yes? So why does not this work:
>
> db = PQconnectdb("postgres://localhost");
>
> ?

Good catch.

I've figured out that we'll need a bit more intrusive change than simply
overriding the expand_dbname check in conninfo_array_parse (like the
current version does) to support URIs in all PQconnect* variants.

I still need to figure out some details, but this is to give people a
status update.

--
Alex

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

> Marko Kreen <markokr(at)gmail(dot)com> writes:
>
>> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>>> https://github.com/a1exsh/postgres/commits/uri
>>
>> The point of the patch is to have one string with all connection options,
>> in standard format, yes? So why does not this work:
>>
>> db = PQconnectdb("postgres://localhost");
>>
>> ?
>
> Good catch.
>
> I've figured out that we'll need a bit more intrusive change than simply
> overriding the expand_dbname check in conninfo_array_parse (like the
> current version does) to support URIs in all PQconnect* variants.
>
> I still need to figure out some details, but this is to give people a
> status update.

While working on this fix I've figured out that I need my
conninfo_uri_parse to support use_defaults parameter, like
conninfo(_array)_parse functions do.

The problem is that the block of code which does the defaults handling
is duplicated in both of the above functions. What I'd like to do is
extract it into a separate function to call. What I wouldn't like is
bloating the original URI patch with this unrelated change.

So here's a trivial patch to do the refactoring. Also, it uses the
newly added conninfo_fill_defaults directly in PQconndefaults, instead
of doing the "parse empty conninfo string" trick. If this could be
applied, I'd rebase my patch against the updated master branch and
submit the new version.

As it goes, the patch adds a little duplication when it comes to
creating a working copy of PQconninfoOptions array. Attached is the
second patch on top of the first one to extract this bit of code into
conninfo_init. Not sure if we should go all the way through this, so
it's not critical if this one is not applied.

--
Regards,
Alex

Alex Shulgin <ash(at)commandprompt(dot)com> writes:
> While working on this fix I've figured out that I need my
> conninfo_uri_parse to support use_defaults parameter, like
> conninfo(_array)_parse functions do.

> The problem is that the block of code which does the defaults handling
> is duplicated in both of the above functions. What I'd like to do is
> extract it into a separate function to call. What I wouldn't like is
> bloating the original URI patch with this unrelated change.

Applied with minor adjustments --- notably, I thought
conninfo_add_defaults was a better name for the function.

regards, tom lane

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

> Marko Kreen <markokr(at)gmail(dot)com> writes:
>
>> On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
>>> https://github.com/a1exsh/postgres/commits/uri
>>
>> The point of the patch is to have one string with all connection options,
>> in standard format, yes? So why does not this work:
>>
>> db = PQconnectdb("postgres://localhost");
>>
>> ?
>
> Good catch.
>
> I've figured out that we'll need a bit more intrusive change than simply
> overriding the expand_dbname check in conninfo_array_parse (like the
> current version does) to support URIs in all PQconnect* variants.

Okay, at last here's v9, rebased against current master branch.

What's new in this version is parse_connection_string function to be
called instead of conninfo_parse. It will check for possible URI in the
connection string and dispatch to conninfo_uri_parse if URI was found,
otherwise it will fall back to conninfo_parse.

In two places in code we don't want to parse the string unless it is
deemed a real connection string and not plain dbname. The new function
recognized_connection_string is added to check this.

Thanks Marko for spotting the problem and thanks Tom for committing the
refactoring patch!

--
Alex

On 22.03.2012 23:42, Alex wrote:
> Okay, at last here's v9, rebased against current master branch.

Some quick comments on this patch:

I see a compiler warning:
fe-connect.c: In function ‘conninfo_parse’:
fe-connect.c:4113: warning: unused variable ‘option’

Docs are missing.

I wonder if you should get an error if you try specify the same option
multiple times. At least the behavior needs to be documented.

Should %00 be forbidden?

The error message is a bit confusing for
"postgres://localhost?dbname=%XXfoo":
WARNING: ignoring unrecognized URI query parameter: dbname
There is in fact nothing wrong with "dbname", it's the %XX in the value
that's bogus.

Looking at conninfo_uri_decode(), I think it's missing a bounds check,
and peeks at two bytes beyond the end of string if the input ends in a '%'.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:

> On 22.03.2012 23:42, Alex wrote:
>> Okay, at last here's v9, rebased against current master branch.
>
> Some quick comments on this patch:

Heikki, thank you for taking a look at this!

> I see a compiler warning:
> fe-connect.c: In function ‘conninfo_parse’:
> fe-connect.c:4113: warning: unused variable ‘option’

The warning is due to the earlier commit e9ce658b. I believe the above
line supposed to go away.

> Docs are missing.

Yes, my plan is to focus on the documentation and code comments while
sorting out any remaining issues with the code.

> I wonder if you should get an error if you try specify the same option
> multiple times. At least the behavior needs to be documented.

Since conninfo strings may contain duplicated keywords and the latter
just takes precedence, I think we should just do the same with URIs
(which we already do.) I don't see the behavior of conninfo strings
documented anywhere, however.

> Should %00 be forbidden?

Probably yes, good spot.

> The error message is a bit confusing for
> "postgres://localhost?dbname=%XXfoo":
> WARNING: ignoring unrecognized URI query parameter: dbname
> There is in fact nothing wrong with "dbname", it's the %XX in the
> value that's bogus.

Hm, yes, that's a bug.

Looks like conninfo_uri_parse_params needs to be adjusted to properly
pass the error message generated by conninfo_store_uri_encoded_value. I
wonder if examining the errorMessage buffer to tell if it's a hard error
(it's going to be empty in the case of ignoreMissing=true) is a good
practice.

> Looking at conninfo_uri_decode(), I think it's missing a bounds check,
> and peeks at two bytes beyond the end of string if the input ends in a
> %'.

No, in that case what happens on L4919 is this: we dereference q which
is pointing at NUL terminator and pass the value to the first
get_hexdigit in the "if" condition, the pointer itself is then
incremented and does point beyond the end of string, but since
get_hexdigit returns FALSE we don't call the second get_hexdigit, so we
don't dereference the invalid pointer.

There is a comment right before that "if" which says just that.

--
Alex

On tor, 2012-03-22 at 23:42 +0200, Alex wrote:
> Okay, at last here's v9, rebased against current master branch.

Attached is a patch on top of your v9 with two small fixes:

- Don't provide a check target in libpq/Makefile if it's not
implemented.

- Use the configured port number for running the tests (otherwise it
runs against my system installation, for example).

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:

> On tor, 2012-03-22 at 23:42 +0200, Alex wrote:
>> Okay, at last here's v9, rebased against current master branch.
>
> Attached is a patch on top of your v9 with two small fixes:
>
> - Don't provide a check target in libpq/Makefile if it's not
> implemented.
>
> - Use the configured port number for running the tests (otherwise it
> runs against my system installation, for example).

Neat, thank you.

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

> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>
>> Attached is a patch on top of your v9 with two small fixes:
>>
>> - Don't provide a check target in libpq/Makefile if it's not
>> implemented.
>>
>> - Use the configured port number for running the tests (otherwise it
>> runs against my system installation, for example).
>
> Neat, thank you.

Attached is a gzipped v10, complete with the above fixes and fixes to
bugs found by Heikki. Documentation and code comments are also finally
updated.

Of all the command-line utilities which can accept conninfo string, only
psql mentions that, so only its documentation was updated. Other
utilities, like pg_dump and pg_restore can also work with either
conninfo or URI, however this remains undocumented.

--
Alex

On ons, 2012-03-28 at 01:49 +0300, Alex wrote:
> Attached is a gzipped v10, complete with the above fixes and fixes to
> bugs found by Heikki. Documentation and code comments are also
> finally updated.

The compiler warning is still there:

fe-connect.c: In function ‘conninfo_parse’:
fe-connect.c:4122:20: error: unused variable ‘option’ [-Werror=unused-variable]

Excerpts from Peter Eisentraut's message of jue abr 05 13:52:13 -0300 2012:
> On ons, 2012-03-28 at 01:49 +0300, Alex wrote:
> > Attached is a gzipped v10, complete with the above fixes and fixes to
> > bugs found by Heikki. Documentation and code comments are also
> > finally updated.
>
> The compiler warning is still there:
>
> fe-connect.c: In function ‘conninfo_parse’:
> fe-connect.c:4122:20: error: unused variable ‘option’ [-Werror=unused-variable]

Here's an updated patch, including this fix and others. (Most notable
the fact that I got rid of conninfo_store_uri_encoded_value, instead
expanding conninfo_storeval a bit). I also fixed the test script to run
in VPATH. I intend to commit this shortly, barring objection from Peter
who is listed as "committer" in the CF app.

I think the only thing I'm not really sure about is the usage of the
<synopsis> tag to mark example URIs in the docs. It looks to me that
they should mostly be <literal> instead, but I'm not really sure about
that either -- I'm suspecting the PDF output would look rather horrible.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of jue abr 05 19:10:25 -0300 2012:

> I think the only thing I'm not really sure about is the usage of the
> <synopsis> tag to mark example URIs in the docs. It looks to me that
> they should mostly be <literal> instead, but I'm not really sure about
> that either -- I'm suspecting the PDF output would look rather horrible.

Some moments of radical thinking later, I became unhappy with the fact
that the conninfo stuff and parameter keywords are all crammed in the
PQconnectdbParams description. This feels wrong to me, even more so
after we expand it even more to add URIs to the mix. I think it's
better to create a separate sect1 (which I've entitled "Connection
Strings") which explains the conninfo and URI formats as well as
accepted keywords. The new section is referenced from the multiple
places that need it, without having to point to PQconnectdbParams.

Thoughts?

Wording suggestions are welcome.

dblink_connect docs should also have a mention of URIs now:

alvherre=# select dblink_connect('postgresql://localhost:55432');
dblink_connect
----------------
OK
(1 fila)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On fre, 2012-04-06 at 00:25 -0300, Alvaro Herrera wrote:
> Some moments of radical thinking later, I became unhappy with the fact
> that the conninfo stuff and parameter keywords are all crammed in the
> PQconnectdbParams description. This feels wrong to me, even more so
> after we expand it even more to add URIs to the mix. I think it's
> better to create a separate sect1 (which I've entitled "Connection
> Strings") which explains the conninfo and URI formats as well as
> accepted keywords. The new section is referenced from the multiple
> places that need it, without having to point to PQconnectdbParams.

Yes, it should be split out. But the libpq chapter already has too many
tiny sect1s. I think it should be a sect2 under "Database Connection
Control".

Excerpts from Peter Eisentraut's message of vie abr 06 03:09:10 -0300 2012:
> On fre, 2012-04-06 at 00:25 -0300, Alvaro Herrera wrote:
> > Some moments of radical thinking later, I became unhappy with the fact
> > that the conninfo stuff and parameter keywords are all crammed in the
> > PQconnectdbParams description. This feels wrong to me, even more so
> > after we expand it even more to add URIs to the mix. I think it's
> > better to create a separate sect1 (which I've entitled "Connection
> > Strings") which explains the conninfo and URI formats as well as
> > accepted keywords. The new section is referenced from the multiple
> > places that need it, without having to point to PQconnectdbParams.
>
> Yes, it should be split out. But the libpq chapter already has too many
> tiny sect1s. I think it should be a sect2 under "Database Connection
> Control".

Thanks, that seems a good idea. I have tweaked things slightly and it
looks pretty decent to me. Wording improvements are welcome. The file
in its entirety can be seen here:
https://github.com/alvherre/postgres/blob/uri/doc/src/sgml/libpq.sgml
The new bits start at line 1224. I also attach the HTML output for easy
reading. (I wonder if it's going to be visible in the archives).

There are three minor things that need to be changed for this to be
committable:

1. it depends on strtok_r which is likely to be a problem in MSVC++ and
perhaps older Unix platforms as well.

2. The ssl=true trick being converted into sslmode=require doesn't work
if the URI specifies them uri-encoded, which seems bogus.

3. if an unknown keyword is uri-encoded, the error message displays it
still uri-encoded. Seems to me it'd be better to uri-decode it before
throwing error.

Alexander says he's going to work on these and then I'll finally commit it.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun abr 09 16:41:50 -0300 2012:

> There are three minor things that need to be changed for this to be
> committable:

Committed this patch after some more editorialization; in particular the
test was rewritten so that instead of trying to connect, it uses
PQconninfoParse to figure out how the URI is parsed, which makes a
lot more sense. Also some other changes to the accepted URI, in
particular so that username, pwd, and port are possible to be specified
when using unix-domain sockets.

Now that it is a done deal I'm sure people will start complaining how
bad the documentation change was; please keep the flames up.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support