From: | Alex Shulgin <ash(at)commandprompt(dot)com> |
---|---|
To: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
Cc: | Marko Kreen <markokr(at)gmail(dot)com>, Daniel Farina <daniel(at)heroku(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Another review of URI for libpq, v7 submission |
Date: | 2012-03-27 14:13:55 |
Message-ID: | 87mx72w2qk.fsf@commandprompt.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2012-03-27 14:29:58 | Re: Command Triggers patch v18 |
Previous Message | Tom Lane | 2012-03-27 13:49:25 | Re: Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role) |