Обсуждение: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2

The following bug has been logged on the website:

Bug reference:      15827
Logged by:          Jorge Gustavo Rocha
Email address:      jgr@geomaster.pt
PostgreSQL version: 10.8
Operating system:   Windows
Description:

Hi,

It is my fist report, so please be gentle. I need some help to identify the
problem properly.

1. Description

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

The pg_services.conf file is:

[pg_trabalho]
host=192.168.1.24
port=5432
dbname=trabalho

The services are working well, either on Ubuntu and on Windows. I'm using
QGIS to access the database and it works well. QGIS is using cpp code to
access the database.

I can also connect on the command line, using:
psql service=pg_trabalho 
as in the following example:

jgr@zoe:~$ psql service=pg_trabalho
psql (10.8 (Ubuntu 10.8-0ubuntu0.18.10.1))
SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384,
bits: 256, compression: off)
Type "help" for help.

trabalho=> 

So, I think the pg_services are properly configured and working.

2. Problem

In Windows, I'm not able to connect using psycopg2. The following code does
not work on Windows.

import psycopg2

print (psycopg2.__version__)
connection = psycopg2.connect(service='pg_trabalho')
# connection = psycopg2.connect(user = "cmb.user", password = "xxxxxxx",
host = "192.168.1.24", port = "5432", database = "trabalho")
cursor = connection.cursor()
print ( connection.get_dsn_parameters(),"\n")
cursor.execute("SELECT version();")
record = cursor.fetchone()
print("Connected to - ", record,"\n")

It works well in Ubuntu, but not on Windows. Both works well if pg_services
are not used, and the connection is made using `connect(user = "cmb.user",
password = "xxxxxxx", host = "192.168.1.24", port = "5432", database =
"trabalho").

3. Error on Windows

This is the error reported in Windows:

2.7.5 (dt dec pq3 ext lo64)
Traceback (most recent call last):
  File "C:\OSGEO4~1\apps\Python37\lib\code.py", line 90, in runcode
    exec(code, self.locals)
  File "<input>", line 1, in <module>
  File "<string>", line 4, in <module>
  File "C:\OSGEO4~1\apps\Python37\lib\site-packages\psycopg2\__init__.py",
line 130, in connect
    conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
psycopg2.OperationalError: could not translate host name "192.168.1.24
" to address: Unknown host

4. What I have tried

I report this as a QGIS problem [1], but soon I've realize that it was a
psycopg2 problem.

I report this as a psycopg2 problem [2], but psycopg2 just passes the
parameters down to libpq library and the issue was closed.

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).

I've tried with the hostname and IP address.

Definitely, it is not a pg_services.conf syntax, because psql also runs fine
on Windows [3]. Print screen is attached.

I'm using psycopg2 2.7.5, on Windows 10.

Who should I do to help better identify the problem? 

[1] https://github.com/qgis/QGIS/issues/30027
[2] https://github.com/psycopg/psycopg2/issues/926
[3] https://gist.github.com/jgrocha/a3a8bc2d2476386450ed4d8d3629fe32

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

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

--

   Geomaster, LDA

  VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSCO

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

-- 

  
                    Geomaster, LDA

  VENHA DESCOBRIR O                      CAMINHO DO OPEN SOURCE CONNOSCO
                      
                                                        

 
                                          Avenida Barros e Soares
                                          N.º 423, 4715-214 Braga
                                          VAT/NIF                                          510 906 109
                                          Phone                                            +351 253 680 323
                                        Site                                              geomaster.pt
                                        GPS                                              41.53322, -8.41929

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

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

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

--

   Geomaster, LDA

  VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSCO

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,

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.

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]. 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

			regards, tom lane

--

   Geomaster, LDA

  VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSCO

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,

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

--

   Geomaster, LDA

  VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSCO

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

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c800d79..f2b166b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -5020,6 +5020,8 @@ parseServiceFile(const char *serviceFile,

     while ((line = fgets(buf, sizeof(buf), f)) != NULL)
     {
+        int            len;
+
         linenr++;

         if (strlen(line) >= sizeof(buf) - 1)
@@ -5032,16 +5034,17 @@ parseServiceFile(const char *serviceFile,
             return 2;
         }

-        /* ignore EOL at end of line */
-        if (strlen(line) && line[strlen(line) - 1] == '\n')
-            line[strlen(line) - 1] = 0;
+        /* ignore whitespace at end of line, especially the newline */
+        len = strlen(line);
+        while (len > 0 && isspace((unsigned char) line[len - 1]))
+            line[--len] = '\0';

-        /* ignore leading blanks */
+        /* ignore leading whitespace too */
         while (*line && isspace((unsigned char) line[0]))
             line++;

         /* ignore comments and empty lines */
-        if (strlen(line) == 0 || line[0] == '#')
+        if (line[0] == '\0' || line[0] == '#')
             continue;

         /* Check for right groupname */
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 877226d..4abbef5 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -112,9 +112,10 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
         goto error;
     }

-    /* strip trailing newline */
+    /* strip trailing newline, including \r in case we're on Windows */
     len = strlen(buf);
-    if (len > 0 && buf[len - 1] == '\n')
+    while (len > 0 && (buf[len - 1] == '\n' ||
+                       buf[len - 1] == '\r'))
         buf[--len] = '\0';

 error:
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index dfb6c19..79c4627 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2176,6 +2176,7 @@ adjust_data_dir(void)
                 filename[MAXPGPATH],
                *my_exec_path;
     FILE       *fd;
+    int            len;

     /* do nothing if we're working without knowledge of data dir */
     if (pg_config == NULL)
@@ -2218,9 +2219,12 @@ adjust_data_dir(void)
     pclose(fd);
     free(my_exec_path);

-    /* Remove trailing newline */
-    if (strchr(filename, '\n') != NULL)
-        *strchr(filename, '\n') = '\0';
+    /* Remove trailing newline, handling Windows newlines as well */
+    len = strlen(filename);
+    while (len > 0 &&
+           (filename[len - 1] == '\n' ||
+            filename[len - 1] == '\r'))
+        filename[--len] = '\0';

     free(pg_data);
     pg_data = pg_strdup(filename);
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 2734f87..256363c 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -559,12 +559,10 @@ CheckDataVersion(void)

     /* remove trailing newline, handling Windows newlines as well */
     len = strlen(rawline);
-    if (len > 0 && rawline[len - 1] == '\n')
-    {
+    while (len > 0 &&
+           (rawline[len - 1] == '\n' ||
+            rawline[len - 1] == '\r'))
         rawline[--len] = '\0';
-        if (len > 0 && rawline[len - 1] == '\r')
-            rawline[--len] = '\0';
-    }

     if (strcmp(rawline, PG_MAJORVERSION) != 0)
     {
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 73f395f..202da23 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -405,6 +405,7 @@ adjust_data_dir(ClusterInfo *cluster)
                 cmd_output[MAX_STRING];
     FILE       *fp,
                *output;
+    int            len;

     /* Initially assume config dir and data dir are the same */
     cluster->pgconfig = pg_strdup(cluster->pgdata);
@@ -445,9 +446,12 @@ adjust_data_dir(ClusterInfo *cluster)

     pclose(output);

-    /* Remove trailing newline */
-    if (strchr(cmd_output, '\n') != NULL)
-        *strchr(cmd_output, '\n') = '\0';
+    /* Remove trailing newline, handling Windows newlines as well */
+    len = strlen(cmd_output);
+    while (len > 0 &&
+           (cmd_output[len - 1] == '\n' ||
+            cmd_output[len - 1] == '\r'))
+        cmd_output[--len] = '\0';

     cluster->pgdata = pg_strdup(cmd_output);

@@ -508,10 +512,15 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
                     sscanf(line, "%hu", &old_cluster.port);
                 if (lineno == LOCK_FILE_LINE_SOCKET_DIR)
                 {
+                    int            len;
+
                     cluster->sockdir = pg_strdup(line);
-                    /* strip off newline */
-                    if (strchr(cluster->sockdir, '\n') != NULL)
-                        *strchr(cluster->sockdir, '\n') = '\0';
+                    /* strip off newline, handling Windows newlines as well */
+                    len = strlen(cluster->sockdir);
+                    while (len > 0 &&
+                           (cluster->sockdir[len - 1] == '\n' ||
+                            cluster->sockdir[len - 1] == '\r'))
+                        cluster->sockdir[--len] = '\0';
                 }
             }
             fclose(fp);
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 4514cf8..59afbc7 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -264,6 +264,7 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
                         FILE       *fd;
                         char       *file = pg_strdup(p + 1);
                         int            cmdend;
+                        int            buflen;

                         cmdend = strcspn(file, "`");
                         file[cmdend] = '\0';
@@ -274,8 +275,10 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
                                 buf[0] = '\0';
                             pclose(fd);
                         }
-                        if (strlen(buf) > 0 && buf[strlen(buf) - 1] == '\n')
-                            buf[strlen(buf) - 1] = '\0';
+                        buflen = strlen(buf);
+                        while (buflen > 0 && (buf[buflen - 1] == '\n' ||
+                                              buf[buflen - 1] == '\r'))
+                            buf[--buflen] = '\0';
                         free(file);
                         p += cmdend + 1;
                         break;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c800d79..f2b166b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6910,14 +6913,10 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,

         len = strlen(buf);

-        /* Remove trailing newline */
-        if (len > 0 && buf[len - 1] == '\n')
-        {
+        /* Remove trailing newline, including \r in case we're on Windows */
+        while (len > 0 && (buf[len - 1] == '\n' ||
+                           buf[len - 1] == '\r'))
             buf[--len] = '\0';
-            /* Handle DOS-style line endings, too, even when not on Windows */
-            if (len > 0 && buf[len - 1] == '\r')
-                buf[--len] = '\0';
-        }

         if (len == 0)
             continue;
diff --git a/src/port/sprompt.c b/src/port/sprompt.c
index 146fb00..02164d4 100644
--- a/src/port/sprompt.c
+++ b/src/port/sprompt.c
@@ -144,9 +144,11 @@ simple_prompt(const char *prompt, char *destination, size_t destlen, bool echo)
         } while (buflen > 0 && buf[buflen - 1] != '\n');
     }

-    if (length > 0 && destination[length - 1] == '\n')
-        /* remove trailing newline */
-        destination[length - 1] = '\0';
+    /* strip trailing newline, including \r in case we're on Windows */
+    while (length > 0 &&
+           (destination[length - 1] == '\n' ||
+            destination[length - 1] == '\r'))
+        destination[--length] = '\0';

     if (!echo)
     {

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

I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> 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.)

Hearing nothing further on this, I went ahead with the patch as I had it
on HEAD, and a tweaked version of your patch on the back branches.

            regards, tom lane

Hi Tom,

Thank you!

Best regards,

Jorge

I wrote:

Michael Paquier <michael@paquier.xyz> writes:

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.)

Hearing nothing further on this, I went ahead with the patch as I had it
on HEAD, and a tweaked version of your patch on the back branches.
			regards, tom lane

--

   Geomaster, LDA

  VENHA DESCOBRIR O CAMINHO DO OPEN SOURCE CONNOSCO

On Thu, Jul 25, 2019 at 12:13:32PM -0400, Tom Lane wrote:
> Hearing nothing further on this, I went ahead with the patch as I had it
> on HEAD, and a tweaked version of your patch on the back branches.

Thanks Tom for taking care of that stuff.  Coming late at the party, I
have one comment post-commit.  Perhaps we could have a wrapper in
charge of doing this work in with something adding properly \0 at the
end of those strings for CR and LF and returning the new length after
adjustment, like pg_strip_clrf in common/string.c?  There are nine
places changed on HEAD.
--
Michael