Horton

Thread

Commits

  1. Fix failures to ignore \r when reading Windows-style newlines.

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

    The Post Office <noreply@postgresql.org> — 2019-05-31T15:57:40Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-01T17:19:23Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-01T17:58:31Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-01T18:06:28Z

    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

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

    Ireneusz Pluta <ipluta@wp.pl> — 2019-06-01T19:51:52Z

    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?

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-18T13:13:01Z

    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

À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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-18T13:54:45Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-18T14:43:10Z

    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>
GPS       41.53322, -8.41929

                                      

                                    


                                      

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

                                    

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

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

    Daniele Varrazzo <daniele.varrazzo@gmail.com> — 2019-06-18T14:54:40Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-19T20:01:49Z

    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>
GPS       41.53322, -8.41929
	
------------------------------------------------------------------------
	 
Jorge Gustavo Rocha
CTO

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-19T20:44:58Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-19T23:19:08Z

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

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-20T03:12:18Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-20T07:03:19Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-06-20T08:48:25Z

    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>
GPS       41.53322, -8.41929
	
------------------------------------------------------------------------
	 
Jorge Gustavo Rocha
CTO

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

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-21T00:42:33Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-21T00:48:33Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-27T16:20:35Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-28T03:10:56Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-28T19:33:31Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-25T16:13:32Z

    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

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

    Jorge Gustavo Rocha <jgr@geomaster.pt> — 2019-07-25T16:43:47Z

    Hi Tom,

Thank you!

Best regards,

Jorge

Às 17:13 de 25/07/19, Tom Lane escreveu:
> 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
-- 
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

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

    Michael Paquier <michael@paquier.xyz> — 2019-07-26T04:58:08Z

    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