Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Jorge Gustavo Rocha <jgr(at)geomaster(dot)pt>, pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
Date: 2019-06-27 16:20:35
Message-ID: 25605.1561652435@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

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

Attachment Content-Type Size
0001-fix-parseServiceFile.patch text/x-diff 1.1 KB
0002-fix-other-places.patch text/x-diff 5.6 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Alvaro Herrera 2019-06-27 16:33:32 Re: BUG #15724: Can't create foreign table as partition
Previous Message Daniel Gustafsson 2019-06-27 12:35:26 Re: BUG #15876: A SUGGESTION