Re: pgsql: Check dup2() results in syslogger

On 01/27/2014 05:32 PM, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> Check dup2() results in syslogger
>>> Consistently check the dup2() call results throughout syslogger.c.
>>> It's pretty unlikely that they'll error out, but if they do,
>>> ereport(FATAL) instead of blissfully continuing on.
>>
>> Meh.  Have you actually tested that an ereport(FATAL) is capable of doing
>> anything sane right there, with so much syslogger initialization left to
>> do, and no working stderr?
>
> Given that we were already doing so later on in the same function, it
> struck me as appropriate.  I agree that the case is a bit interesting to
> consider.
>
>> Please note also that the comment just above
>> this implies that we are deliberately ignoring any failures here, so I
>> think FATAL was probably the wrong thing in any case.
>
> We were explicitly ignoring the errors from the close(), I don't believe
> those comments were intended to apply to the dup2() calls as well, based
> on my reading of the commit history.
>
>>> Spotted by the Coverity scanner.
>>
>> I fear this is mere Coverity-appeasement that has broken code that used
>> to work.
>
> Those dup2() calls look likely to only fail in a case of running out of
> file handles or similar, which struck me as a good reason to
> ereport(FATAL), similar to the setsid() failure which is checked for
> below.  I could have just done (void) dup2() instead to make it clear
> that we were intentionally ignoring the results to please Coverity, and
> probably should have adjusted the close() calls that way.
>
> The last adjustment to this code was actually to avoid the dup2() calls
> causing failures *regardless* of our ignoring the result code due to a
> Windows' feature in CRT called Paramter Validation (per Heikki's commit
> message).  Heikki, any thoughts on the right answer here?  I admit that
> I didn't go to the effort of forcing the dup2() calls to fail to see
> what happens, though I did play around w/ killing off the syslogger
> forcibly and making sure it came back, which appeared to work fine.

Parameter Validation makes it unsafe to pass an invalid file handle as
argument to a function, like dup2. That's not what was happening here.

It would be good to test what happens if you force that FATAL to happen.
On Windows, too - there's plenty of platform-dependent stuff happening
there.

- Heikki