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

PG Bug reporting form <noreply@postgresql.org> writes:

I'm using `pg_services.conf` to provide access to a Postgresql database.
...
From other's feedback, I've tried to use the line separators in
pg_services.conf with \n and \r\n, but the result is the same (because there
is strange newline after the host address).

Hm, can you double check that? It sure looks like something is failing to
remove the "\r" from that line of pg_service.conf. Which is odd, because
libpq opens the file in text mode so the Windows C library ought to take
care of reducing "\r\n" to "\n".

It seems like in general, maybe we ought to trim trailing spaces from
pg_service.conf entries automatically. But I'm not entirely sure if
that would fix this problem...

regards, tom lane

Hi Tom,

Thanks for looking at this.

I've tried with \n and \r\n separators to make sure it wasn't the
problem. I did a od -c to make sure the endings were the expected ones,
and I found no difference between just \n or \r\n.

The psql is able to deal with service=pg_trabalho, so I think libpq is
reading the file without line end issues.

I was able to get it run on another Windows (on a virtual machine I've
installed).

Right now, I suspected that the problem might be even upstream, when
libpq tries to use/resolve the address. I have to return to the network
where the problem was reported (a large intranet) and test it in the
original environment.

Regards,

Jorge Gustavo

Às 18:19 de 01/06/19, Tom Lane escreveu:

PG Bug reporting form <noreply@postgresql.org> writes:

I'm using `pg_services.conf` to provide access to a Postgresql database.
...
From other's feedback, I've tried to use the line separators in
pg_services.conf with \n and \r\n, but the result is the same (because there
is strange newline after the host address).

Hm, can you double check that? It sure looks like something is failing to
remove the "\r" from that line of pg_service.conf. Which is odd, because
libpq opens the file in text mode so the Windows C library ought to take
care of reducing "\r\n" to "\n".

It seems like in general, maybe we ought to trim trailing spaces from
pg_service.conf entries automatically. But I'm not entirely sure if
that would fix this problem...

regards, tom lane

--
Logo*   Geomaster, LDA*
  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O

*
 
Avenida Barros e Soares
N.º 423, 4715-214 Braga
VAT/NIF510 906 109
Phone  +351 253 680 323
Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------
 
Jorge Gustavo Rocha
CTO

Mobile  +351 910 333 888
Email    jgr@geomaster.pt

Jorge Gustavo Rocha <jgr@geomaster.pt> writes:

Hi Tom,
Thanks for looking at this.

I've tried with \n and \r\n separators to make sure it wasn't the
problem. I did a od -c to make sure the endings were the expected ones,
and I found no difference between just \n or \r\n.

The psql is able to deal with service=pg_trabalho, so I think libpq is
reading the file without line end issues.

Yeah, that seems like it puts libpq in the clear, but then where are
things going wrong? I wonder if psycopg2 is not just "passing the
connection parameters down to libpq", but is reading the service
file for itself (and failing to take care about newlines).

regards, tom lane

W dniu 2019-05-31 o 17:57, PG Bug reporting form pisze:

I'm using `pg_services.conf` to provide access to a Postgresql database.

correct name of the services file is "pg_service.conf", not "pg_services.conf" (unless you change it
with $PGSERVICEFILE).
Are you sure you're touching the right file?

Dear Tom Lane,

Thank you for your feedback.

The '\r' on pg_services.conf is causing problems on Windows. The
parseServiceFile function returns the host or hostaddr with a trialing
'\r'. Subsequent attempts to turn that into an address will fail.

I've checked the code, and parseServiceFile uses the standard C fgets
library function. Since fgets copies all characters until '\n'
(including the '\n'), the resulting line (right now) preserves the '\r'
at the end, on Windows.

Since parseServiceFile already checks for the trailing '\n' and removes
it, I think we can also check for a trailing '\r' and remove it. So, I
suggest to improve the parseServiceFile function [1]src/interfaces/libpq/fe-connect.c to discard trailing
'\r'.

I've isolated the problem, and I think the attached patch solves this
issue. The added filter for '\r' has no effect if the pg_services file
does not have any '\r'.

I've saw many people complaining of this tiny issue and I think it is
easy to solve.

What do you think? Do you see any inconvenient in filtering '\r'?

Best regards,

Gustavo

[1]: src/interfaces/libpq/fe-connect.c

Às 18:19 de 01/06/19, Tom Lane escreveu:

PG Bug reporting form <noreply@postgresql.org> writes:

I'm using `pg_services.conf` to provide access to a Postgresql database.
...
From other's feedback, I've tried to use the line separators in
pg_services.conf with \n and \r\n, but the result is the same (because there
is strange newline after the host address).

Hm, can you double check that? It sure looks like something is failing to
remove the "\r" from that line of pg_service.conf. Which is odd, because
libpq opens the file in text mode so the Windows C library ought to take
care of reducing "\r\n" to "\n".

It seems like in general, maybe we ought to trim trailing spaces from
pg_service.conf entries automatically. But I'm not entirely sure if
that would fix this problem...

regards, tom lane

--
Logo*   Geomaster, LDA*
  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O

*
 
Avenida Barros e Soares
N.º 423, 4715-214 Braga
VAT/NIF510 906 109
Phone  +351 253 680 323
Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------
 
Jorge Gustavo Rocha
CTO

Mobile  +351 910 333 888
Email    jgr@geomaster.pt

Jorge Gustavo Rocha <jgr@geomaster.pt> writes:

The '\r' on pg_services.conf is causing problems on Windows. The
parseServiceFile function returns the host or hostaddr with a trialing
'\r'. Subsequent attempts to turn that into an address will fail.

So it would seem.

I've checked the code, and parseServiceFile uses the standard C fgets
library function. Since fgets copies all characters until '\n'
(including the '\n'), the resulting line (right now) preserves the '\r'
at the end, on Windows.

Well, that's exactly the question at issue: doesn't Windows' fgets()
convert \r\n to just \n? I should think that it generally does, because
we have a *lot* of fgets() calls and a quick scan says that the majority
of them aren't taking care to get rid of \r. If you can convince me that
this is actually a behavior seen in the wild, we're going to need to
change way more places than just this one.

Googling for this didn't provide a lot of insight, although I did find
one person speculating that if you used GNU glibc on Windows it would not
strip \r. That seems unlikely though.

Another possibility is that you're on a Unix machine but you're wishing
libpq would deal with a service file that has Windows-style newlines.

Anyway, I want some clarity about what's really happening here, because
I'm disinclined to touch several dozen call sites on the basis of
speculation.

I've saw many people complaining of this tiny issue and I think it is
easy to solve.

Nobody else has complained of this that I've heard of. Please let's
deal in verifiable facts.

regards, tom lane

Hi Tom,

Thank you for the feedback.

Às 14:54 de 18/06/19, Tom Lane escreveu:

Jorge Gustavo Rocha <jgr@geomaster.pt> writes:

The '\r' on pg_services.conf is causing problems on Windows. The
parseServiceFile function returns the host or hostaddr with a trialing
'\r'. Subsequent attempts to turn that into an address will fail.

So it would seem.

I've checked the code, and parseServiceFile uses the standard C fgets
library function. Since fgets copies all characters until '\n'
(including the '\n'), the resulting line (right now) preserves the '\r'
at the end, on Windows.

Well, that's exactly the question at issue: doesn't Windows' fgets()
convert \r\n to just \n? I should think that it generally does, because
we have a *lot* of fgets() calls and a quick scan says that the majority
of them aren't taking care to get rid of \r. If you can convince me that
this is actually a behavior seen in the wild, we're going to need to
change way more places than just this one.

I think it depends on the C library implementation. I've checked this discussion on SO: https://stackoverflow.com/questions/12769289/carriage-return-by-fgets

One answer is related to the C standard and another says that Microsoft compilers behave differently. So, it depends on how libpq is compiled. Does it make sense?

I found that, in Windows, after installing QGIS, which includes libpq.dll, the library does not discard '\r'. Maybe QGIS for Windows is compiled with a non Microsoft compiler. I've cc Jürgen Fischer that might provide more details.

||||

|If everybody uses the same compiler and all get the '\r' removed from
text files, I agree that we don't need to change anything. But if there
are compilers available, implementing the C standard and don't strip
automatically '\r', at least we should warn users that these compilers
can produce code with this small limitation. |

Googling for this didn't provide a lot of insight, although I did find
one person speculating that if you used GNU glibc on Windows it would not
strip \r. That seems unlikely though.

Another possibility is that you're on a Unix machine but you're wishing
libpq would deal with a service file that has Windows-style newlines.

Anyway, I want some clarity about what's really happening here, because
I'm disinclined to touch several dozen call sites on the basis of
speculation.

I've saw many people complaining of this tiny issue and I think it is
easy to solve.

Nobody else has complained of this that I've heard of. Please let's
deal in verifiable facts.

If you check my initial QGIS bug report https://github.com/qgis/QGIS/issues/30027 there is *something* in front of the host address:

|psycopg2.OperationalError: could not translate host name "192.168.1.24
" to address: Unknown host|

You can see replies related to the '\r' issue.

1) https://github.com/qgis/QGIS/issues/30027#issuecomment-497433789

2) https://github.com/qgis/QGIS/issues/30027#issuecomment-498690261

3) https://github.com/qgis/QGIS/issues/30027#issuecomment-498700090

4) https://github.com/qgis/QGIS/issues/30027#issuecomment-501799219

I didn't invented the '\r' problem. I've just jumped into it.

I didn't found any other issue with line endings problems in Postgresql. Maybe other '\r' are not harmful. But these in front of host names or host addresses are critical to resolve the ip addresses.

But, for the sake of clarity, the summary is this:

Installing QGIS, in Windows, with libpq, if the pg_services.conf file has '\r\n' line endings, the pg_services fails.

Installing QGIS, in Windows, with libpq, if the pg_services.conf file only has '\n' line endings, the pg_services rocks!

regards, tom lane

--

Logo*   Geomaster, LDA*

  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O *

  Avenida Barros e Soares N.º 423, 4715-214 Braga VAT/NIF510 906 109
Phone  +351 253 680 323 Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------

  Jorge Gustavo Rocha CTO Mobile  +351 910 333 888 Email    jgr@geomaster.pt

On Tue, Jun 18, 2019 at 3:43 PM Jorge Gustavo Rocha <jgr@geomaster.pt>
wrote:

psycopg2.OperationalError: could not translate host name "192.168.1.24
" to address: Unknown host

You can see replies related to the '\r' issue.
1) https://github.com/qgis/QGIS/issues/30027#issuecomment-497433789

2) https://github.com/qgis/QGIS/issues/30027#issuecomment-498690261

3) https://github.com/qgis/QGIS/issues/30027#issuecomment-498700090

4) https://github.com/qgis/QGIS/issues/30027#issuecomment-501799219

I didn't invented the '\r' problem. I've just jumped into it.

I didn't found any other issue with line endings problems in Postgresql. Maybe other '\r' are not harmful. But these in front of host names or host addresses are critical to resolve the ip addresses.

But, for the sake of clarity, the summary is this:

Installing QGIS, in Windows, with libpq, if the pg_services.conf file has '\r\n' line endings, the pg_services fails.

Installing QGIS, in Windows, with libpq, if the pg_services.conf file only has '\n' line endings, the pg_services rocks!

In all likelyhood, if you are using psycopg on windows, you are using a

libpq compiled for the client, not the libpq shipped with postgres server
for windows.

Compiling the libpq happens in this script:

https://github.com/psycopg/psycopg2/blob/master/scripts/appveyor.py

you can verify if the right compiler and libraries are used, or things are
used in a way that '\r' is not handled correctly.

-- Daniele

Hi Tom,

Just an update on this problem. I'm grateful to the valuable comments by
Daniele Varrazzo, Jason Erickson and Jürgen Fischer.

I confirm that MSC behavior is to discard \r if the file is open in text
mode.

The error I was facing in QGIS is because the application changes the
default fopen mode to binary mode. So, every file is opened in binary
mode and so is pg_services.conf.

So, in this specific usage of libpq, used in another application, we
lost the expected behavior of discarding \r characters.

In my humble opinion, we could make pg_services.conf agnostic in terms
of \r, just by filtering them, if present.

By filtering \r, we make libpq agnostic in terms of line terminators and
also agnostic in terms of compiler. libpq will read pg_services.conf
files with or without \r and compiled with any compiler.

I don't have any more arguments to add.

Thanks for all that helped to identified the problem.

Regards,

Jorge Gustavo

Às 15:54 de 18/06/19, Daniele Varrazzo escreveu:

On Tue, Jun 18, 2019 at 3:43 PM Jorge Gustavo Rocha <jgr@geomaster.pt
<mailto:jgr@geomaster.pt>> wrote:
 

|psycopg2.OperationalError: could not translate host name
"192.168.1.24 " to address: Unknown host|

You can see replies related to the '\r' issue.
1) https://github.com/qgis/QGIS/issues/30027#issuecomment-497433789

2) https://github.com/qgis/QGIS/issues/30027#issuecomment-498690261

3) https://github.com/qgis/QGIS/issues/30027#issuecomment-498700090

4) https://github.com/qgis/QGIS/issues/30027#issuecomment-501799219

I didn't invented the '\r' problem. I've just jumped into it.

I didn't found any other issue with line endings problems in Postgresql. Maybe other '\r' are not harmful. But these in front of host names or host addresses are critical to resolve the ip addresses.

But, for the sake of clarity, the summary is this:

Installing QGIS, in Windows, with libpq, if the pg_services.conf file has '\r\n' line endings, the pg_services fails.

Installing QGIS, in Windows, with libpq, if the pg_services.conf file only has '\n' line endings, the pg_services rocks!

In all likelyhood, if you are using psycopg on windows, you are using
a libpq compiled for the client, not the libpq shipped with postgres
server for windows.

Compiling the libpq happens in this script:

    https://github.com/psycopg/psycopg2/blob/master/scripts/appveyor.py

you can verify if the right compiler and libraries are used, or things
are used in a way that '\r' is not handled correctly.

-- Daniele

--
Logo*   Geomaster, LDA*
  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O

*
 
Avenida Barros e Soares
N.º 423, 4715-214 Braga
VAT/NIF510 906 109
Phone  +351 253 680 323
Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------
 
Jorge Gustavo Rocha
CTO

Mobile  +351 910 333 888
Email    jgr@geomaster.pt

Jorge Gustavo Rocha <jgr@geomaster.pt> writes:

The error I was facing in QGIS is because the application changes the
default fopen mode to binary mode. So, every file is opened in binary
mode and so is pg_services.conf.

Ah-hah! That explains much. So a non-invasive fix would be to force
parseServiceFile to open the file in text mode; I think this would
work for that:

-	f = fopen(serviceFile, "r");
+	f = fopen(serviceFile, "rt");

I don't think that that's really a good solution, because it does little
to prevent the same problem from reappearing elsewhere. But if we wanted
the smallest possible fix that might do.

In my humble opinion, we could make pg_services.conf agnostic in terms
of \r, just by filtering them, if present.

I think we should have it go further and ignore any trailing isspace()
characters, so that trailing spaces don't cause problems either.
But in any case, the real stumbling block here is to get a project-wide
convention as to how to do it.

regards, tom lane

Hi Tom,

Às 21:44 de 19/06/19, Tom Lane escreveu:

Jorge Gustavo Rocha <jgr@geomaster.pt> writes:

The error I was facing in QGIS is because the application changes the
default fopen mode to binary mode. So, every file is opened in binary
mode and so is pg_services.conf.

Ah-hah! That explains much. So a non-invasive fix would be to force
parseServiceFile to open the file in text mode; I think this would
work for that:

-	f = fopen(serviceFile, "r");
+	f = fopen(serviceFile, "rt");

I've wrote a small test program in Windows, using MSVS, and your fix
works as expected. Even if we set the global mode to binary, if the file
is opened with "rt", the file is opened in text mode and \r are
discarded. Good!

I've attached the C file and one pg_services.conf file with \r, just for
the record.

This fixes the problem in QGIS.

I don't think that that's really a good solution, because it does little
to prevent the same problem from reappearing elsewhere. But if we wanted
the smallest possible fix that might do.

I agree with you. It is the smallest possible fix.

In my humble opinion, we could make pg_services.conf agnostic in terms
of \r, just by filtering them, if present.

I think we should have it go further and ignore any trailing isspace()
characters, so that trailing spaces don't cause problems either.
But in any case, the real stumbling block here is to get a project-wide
convention as to how to do it.

Probably this can be accomplish by using a common function to read all
these kind of files. The function handles the text tokenize/parsing and
provided the contents in higher level data structure.

For example, OpenSceneGraph provides wrappers for all functions related
to the file system and to read and write all kind of file formats [1]http://public.vrac.iastate.edu/vancegroup/docs/OpenSceneGraphReferenceDocs-3.0/a01245.html#a27021fc08b8d4088a72e83b81d771481.
Developers can use things like readImageFile, readShaderFile,
readXmlFile, etc, without caring about os type, character encoding, etc.

If you point me some more examples where we need to read such files in
Postgresql, I can think about a proposal to handle all these cases.

But since we are discussing just a small tiny fix, if you agree, we
could apply this fix and later think about a common approach to handle
all text parse needed in different parts of the code. Is this fair?

Regards,

Jorge Gustavo

[1]: http://public.vrac.iastate.edu/vancegroup/docs/OpenSceneGraphReferenceDocs-3.0/a01245.html#a27021fc08b8d4088a72e83b81d771481
http://public.vrac.iastate.edu/vancegroup/docs/OpenSceneGraphReferenceDocs-3.0/a01245.html#a27021fc08b8d4088a72e83b81d771481

regards, tom lane

--
Logo*   Geomaster, LDA*
  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O

*
 
Avenida Barros e Soares
N.º 423, 4715-214 Braga
VAT/NIF510 906 109
Phone  +351 253 680 323
Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------
 
Jorge Gustavo Rocha
CTO

Mobile  +351 910 333 888
Email    jgr@geomaster.pt

On Wed, Jun 19, 2019 at 04:44:58PM -0400, Tom Lane wrote:

Ah-hah! That explains much. So a non-invasive fix would be to force
parseServiceFile to open the file in text mode; I think this would
work for that:

-	f = fopen(serviceFile, "r");
+	f = fopen(serviceFile, "rt");

I don't think that that's really a good solution, because it does little
to prevent the same problem from reappearing elsewhere. But if we wanted
the smallest possible fix that might do.

Actually I think that something like 40cfe86 may be a solution to look
at (combined with 0ba06e0 except for the portions enforcing the text
flags like in initdb). This stuff found its way only in 12~, but
having us switching to the concurrent-safe version for fopen() that we
bundle in port/ has the advantage to take care of this issue, because
on HEAD, if 'b' or 't' are not defined, then we enforce to text mode.
Still back-patching that is sensitive and we have known that hard last
year..

The discussion here is on 10, and the password file has does not
enforce the flag either because it filters '\r' by itself. So another
solution may be to do the same thing as what's done in
passwordFromFile()?
--
Michael

On Thu, Jun 20, 2019 at 12:12:18PM +0900, Michael Paquier wrote:

The discussion here is on 10, and the password file has does not
enforce the flag either because it filters '\r' by itself. So another
solution may be to do the same thing as what's done in
passwordFromFile()?

That still sounds like a good idea to do at the end. So attached is
the Dr Evil's version I was thinking of, and that would be rather
back-patchable. (Spoiler: not seriously tested.)

Thoughts?
--
Michael

Hi Michael,

Às 08:03 de 20/06/19, Michael Paquier escreveu:

On Thu, Jun 20, 2019 at 12:12:18PM +0900, Michael Paquier wrote:

The discussion here is on 10, and the password file has does not
enforce the flag either because it filters '\r' by itself. So another
solution may be to do the same thing as what's done in
passwordFromFile()?

That still sounds like a good idea to do at the end. So attached is
the Dr Evil's version I was thinking of, and that would be rather
back-patchable. (Spoiler: not seriously tested.)

Thoughts?

I've tested your patch and it works. I've just did a small test in
Ubuntu and Windows using MS VSC 2019.

Your version is compiler independent, while Tom's patch was still
relying on MSC fgets behavior (stripping \r if the file is opened in
text mode).

Both patches works and fixes the problem, but this one is compiler
independent.

If this can be back ported, it would be perfect!

Thanks for your patch.

Regards,

Jorge Gustavo

--
Michael

--
Logo*   Geomaster, LDA*
  *VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSC**O

*
 
Avenida Barros e Soares
N.º 423, 4715-214 Braga
VAT/NIF510 906 109
Phone  +351 253 680 323
Site       geomaster.pt <http://geomaster.pt&gt;
GPS       41.53322, -8.41929

------------------------------------------------------------------------
 
Jorge Gustavo Rocha
CTO

Mobile  +351 910 333 888
Email    jgr@geomaster.pt

On Thu, Jun 20, 2019 at 09:48:25AM +0100, Jorge Gustavo Rocha wrote:

Both patches works and fixes the problem, but this one is compiler
independent.

If this can be back ported, it would be perfect!

Thanks for testing. It seems to me that the current behavior is just
annoying, so there is a good argument for back-patching. Now it is
true that we have had few complaints on the matter over the years, and
usually in those cases we bother only about HEAD. I would still do a
back-patch in this case. Any Thoughts from others?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

Thanks for testing. It seems to me that the current behavior is just
annoying, so there is a good argument for back-patching. Now it is
true that we have had few complaints on the matter over the years, and
usually in those cases we bother only about HEAD. I would still do a
back-patch in this case. Any Thoughts from others?

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.

regards, tom lane

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

On Thu, Jun 27, 2019 at 12:20:35PM -0400, Tom Lane wrote:

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.

Yeah, I am not really convinced to add the filtering of lines with
only spaces on back-branches. Nobody has complained about that being
a problem either for years. No objections for HEAD.

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.

InitPostmasterChild() initializes _setmode() to binary, which reacts
on popen() as well, so there is no magic conversion LF => CRLF like
what text does, so your patch looks good to me as we want to be able
to handle the case of external files written in text mode (like the
SSL passphrase case, good catch!).

The part for pg_resetwal does not seem necessary to me. The file gets
written by initdb in binary mode, so there should never be a CR,
right? Or is it worth worrying about custom tools writing the file,
which makes no actual sense normally?

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.

That would basically break a bunch of cases where \r is used in
strings, no, like passwords for single_prompt()? I really think that
we should stick with the approach of only removing \r when it is
followed by \n as we basically want to be able to counter the text
mode of Windows when something external wrote files read by our code,
where \n has been magically transformed to \r\n.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Thu, Jun 27, 2019 at 12:20:35PM -0400, Tom Lane wrote:

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.

The part for pg_resetwal does not seem necessary to me. The file gets
written by initdb in binary mode, so there should never be a CR,
right? Or is it worth worrying about custom tools writing the file,
which makes no actual sense normally?

Well, as I said, some of these changes may not be fixing live bugs.
The idea was just to make all of these code fragments look alike,
rather than guess about which ones might be exposed to the issue.
If we try to filter \r only where really necessary, we're going to
make mistakes down the line because somebody will copy-and-paste
the wrong version of this logic.

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.

That would basically break a bunch of cases where \r is used in
strings, no, like passwords for single_prompt()?

Huh? This is dealing with input only, not output.

I really think that
we should stick with the approach of only removing \r when it is
followed by \n as we basically want to be able to counter the text
mode of Windows when something external wrote files read by our code,
where \n has been magically transformed to \r\n.

As I said, I'm not convinced that filtering \r only where it's actually
adjacent to \n is sufficient, even on Windows. To suppose that it is
sufficient, you'd have to assume that fgets() guarantees not to split
the \r and \n across buffer boundaries, which I doubt that it does.
(If it does do that, it would break some other assumptions we have about
whether the buffer gets filled completely.)

regards, tom lane