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 ‘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
