agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Alex Shulgin <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Marko Kreen <[email protected]>
Cc: Daniel Farina <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Another review of URI for libpq, v7 submission
Date: Tue, 27 Mar 2012 17:13:55 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAAZKuFZ6d5aB8Tt-vq=xdCA4xEcLTDYK_d+G2VunpSuuqyKktA@mail.gmail.com>
<[email protected]>
<CAAZKuFYKZbXhFyXiQOHx87Qq7MbzhN2iaqN_SFCQu3fkCV6Z7Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Heikki Linnakangas <[email protected]> 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
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Another review of URI for libpq, v7 submission
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox