Heikki Linnakangas <heikki.linnakan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
