Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Jorge Gustavo Rocha <jgr@geomaster.pt>, pgsql-bugs@lists.postgresql.org
Date: 2019-06-27T16:20:35Z
Lists: pgsql-bugs
Attachments
- 0001-fix-parseServiceFile.patch (text/x-diff) patch 0001
- 0002-fix-other-places.patch (text/x-diff) patch 0002
I wrote: > I'm still of the opinion that > (1) it's very weird that this code allows for leading space on a line > but not trailing space; > (2) we need to look for other places where we have the same issue. > Possibly libpq is the only chunk of our code that's at serious risk, > since we don't change the default binary mode in the backend. But > even if you assume that that's true, this isn't the only config file > that libpq examines. Patch 0001 attached responds to point (1), ie it uses isspace() tests to get rid of \r and \n and any trailing whitespace in parseServiceFile(). I think we should do this in HEAD, but there's perhaps an argument to be made that this is a behavior change and it'd be better to use Michael's patch in the back branches. For point (2), I looked through all other fgets() callers in our code. Not all of them have newline-chomping logic, but I made all the ones that do have such do it the same way (except for those that use the isspace() method, which is fine). I'm not sure if this is fixing any live bugs --- most of these places are reading popen() output, and it's unclear to me whether we can rely on that to suppress \r on Windows. The Windows-specific code in pipe_read_line seems to think not (but if its test were dead code we wouldn't know it); yet if this were a hazard you'd think we'd have gotten complaints about at least one of these places. Still, I dislike code that has three or four randomly different ways of doing the exact same thing, especially when some of them are gratuitously inefficient. Note that I standardized on a loop that chomps trailing \r and \n indiscriminately, not the "if chomp \n then chomp \r" approach we had in some places. I think that approach does have a corner-case bug: if the input is long enough that the \r fits into the buffer but the \n doesn't, it'd fail to chomp the \r. Thoughts? regards, tom lane
Commits
-
Fix failures to ignore \r when reading Windows-style newlines.
- 8c52b77ddea7 9.4.24 landed
- 6266654db7fa 9.5.19 landed
- ba27151d1eb0 9.6.15 landed
- cb65b454f87b 10.10 landed
- 11a0a4db3cdb 11.5 landed
- c58cf97f2fc4 12.0 landed
- b654714f9bcf 13.0 landed