Date: Fri, 28 Jun 2019 12:10:56 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jorge Gustavo Rocha <jgr@geomaster.pt>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf
 using Python psycopg2
Message-ID: <20190628031056.GA1797@paquier.xyz>
References: <fcb5c34c-cf5f-6dc9-c68e-6b297f073b79@geomaster.pt>
 <CA+mi_8aBnLKypX2Ttksg8JEhApBo=JpF6P_STQGp-FOvDpn6Yw@mail.gmail.com>
 <81d9d421-6947-7111-a1dc-cfd99cd3ae53@geomaster.pt>
 <23215.1560977098@sss.pgh.pa.us>
 <20190620031218.GC2660@paquier.xyz>
 <20190620070319.GH2660@paquier.xyz>
 <0a9fcd30-5ae2-7e5f-ac1d-60d14a2ebb63@geomaster.pt>
 <20190621004233.GA1626@paquier.xyz>
 <3383.1561078113@sss.pgh.pa.us>
 <25605.1561652435@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ew6BAiZeqk4r7MaW"
Content-Disposition: inline
In-Reply-To: <25605.1561652435@sss.pgh.pa.us>
Precedence: bulk


--ew6BAiZeqk4r7MaW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--ew6BAiZeqk4r7MaW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl0VhUAACgkQnvQgOdby
QH3BRQ/+J7l2IpFMVT1wfxUMTH6/dedEXeLOzniZTQ4GW3/m1f8/PR95g9/yFZrc
WfuVhn8hYPvsj23SzB0F9OxfoLY2UtVgGWH8V50XHnq5xE8sgiPzRWFvnuBUsrhL
HexMkKfUNhe5yYME/ieGn/ggy3cjPU3abIcXYvPQ88fHrLXNKX4cCsYFvNnPvBrW
S80YUxAyJErH4oHXYQQVyG20sxMV1LNu843EXYHuWnnVtxVv2YYVsBS9s9bc06yH
G+xVJg1v/45d4/RHmlkT7lX/SpkNl7c9Etx7HvqCqYA3u9CWsuKVj+olxOqPVywV
qsO/ssXzq93jP+PJ1pOGjPDHejMstz5ye5Q9RhNZV+j6QoGWJRkHwtTNOtPzlNqP
wbV8oORil4gQwrK3NUJTKLTL36CaXVrpD3K8ALs7zX3tGgo1CzJdEvUBxSyAr2fw
dA4rwy/M05ymYr93iKaaRY2A7eo4ht1Z3eYMjHiwUS4fOd2B5HkCe7Sij7EEG5sf
bAwFFmiBYL2aR0Hbbl1stHUk/6cCovlgUORbaMS4hTrFa8J0LOh+cOlkw4FA4MET
OQbTqY+VnDGhYicTO1F0QhdDuyfI6ikCdhN01YOv69v2XxWziAmIBSCxDAK98QUP
hG/y9SYzeJkyhblsx2hiGe414z5G04Wg+rC6VUGlqirqd6A19J4=
=F/tT
-----END PGP SIGNATURE-----

--ew6BAiZeqk4r7MaW--