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