From: Alex Shulgin <ash@commandprompt.com>
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: Marko Kreen <markokr@gmail.com>, Daniel Farina <daniel@heroku.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Another review of URI for libpq, v7 submission
References: 
 <CAAZKuFZ6d5aB8Tt-vq=xdCA4xEcLTDYK_d+G2VunpSuuqyKktA@mail.gmail.com>
	<87d38eaq3f.fsf@commandprompt.com>
	<CAAZKuFYKZbXhFyXiQOHx87Qq7MbzhN2iaqN_SFCQu3fkCV6Z7Q@mail.gmail.com>
	<87k42lsgck.fsf@commandprompt.com> <20120317145110.GA25172@gmail.com>
	<8762dyuda3.fsf@commandprompt.com> <87k42cb9dr.fsf@commandprompt.com>
	<4F71BAA2.7080902@enterprisedb.com>
Date: Tue, 27 Mar 2012 17:13:55 +0300
In-Reply-To: <4F71BAA2.7080902@enterprisedb.com> (Heikki Linnakangas's message
	of "Tue, 27 Mar 2012 16:03:30 +0300")
Message-ID: <87mx72w2qk.fsf@commandprompt.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


Heikki Linnakangas <heikki.linnakangas@enterprisedb.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 =E2=80=98conninfo_parse=E2=80=99:
> fe-connect.c:4113: warning: unused variable =E2=80=98option=E2=80=99

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=3D%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=3Dtrue) 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