Thread: pgsql: Check dup2() results in syslogger
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. Spotted by the Coverity scanner. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/790eaa699e4a9626d8a610ec5844e1fd70d73b4e Modified Files -------------- src/backend/postmaster/syslogger.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
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? 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.
> Spotted by the Coverity scanner.
I fear this is mere Coverity-appeasement that has broken code that used
to work.
regards, tom lane
* 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.
Thanks,
Stephen
Attachment
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
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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.
You shouldn't really raise that argument against the guy who made the
original commit in question ;-).
After re-reading a bit, the code here is indeed intentionally ignoring any
failure. The "if (redirection_done)" code block only gets executed in a
second or later incarnation of the syslogger. In the first (and usually
only) incarnation, syslogger inherits the postmaster's original stderr,
which we can hope points to somewhere worth bleating on, so we leave
stderr alone and make sure to write any syslogger-generated messages there
(see for instance the write_stderr call in write_syslogger_file).
However, if that incarnation dies and we have to spawn another, the second
and later incarnations will inherit a postmaster stderr that's already
redirected into the syslogger's input pipe. It is clearly a bad idea for
syslogger to try to write any bleats there, so we forcibly disconnect its
stderr in this case, acknowledging that that makes for a decrease in the
probability that we'll be able to report syslogger problems anywhere that
anybody could see them :-(.
Now ideally, the way we do that is to reconnect its stderr to /dev/null,
but if either the open(DEVNULL) or the dup2() calls were to fail, it will
presumably still work to leave the file descriptors just-plain-closed.
If the syslogger process then attempts to write on stderr, libc's internal
write() calls will fail, but so what? We wanted the output to go to the
bit bucket anyhow.
It's possible that under Windows "parameter validation", such a write
attempt would lead to a process abort. But I'm not sure how a forced
abort during startup is better than a possible future abort after a
failure that shouldn't happen in normal operation.
In short, this patch was ill considered. Please revert. If we need
to silence a Coverity complaint, perhaps a cast-to-void will do?
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> You shouldn't really raise that argument against the guy who made the
> original commit in question ;-).
Figures. :) Not sure how I missed that.
[...]
Right, I had followed that.
> Now ideally, the way we do that is to reconnect its stderr to /dev/null,
> but if either the open(DEVNULL) or the dup2() calls were to fail, it will
> presumably still work to leave the file descriptors just-plain-closed.
> If the syslogger process then attempts to write on stderr, libc's internal
> write() calls will fail, but so what? We wanted the output to go to the
> bit bucket anyhow.
Ok, I see your point that it wouldn't much matter if we tried to
complain at this point about the dup2() calls failing.
> In short, this patch was ill considered. Please revert. If we need
> to silence a Coverity complaint, perhaps a cast-to-void will do?
Sure, I'll adjust it accordingly.
Thanks,
Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> In short, this patch was ill considered. Please revert. If we need
>> to silence a Coverity complaint, perhaps a cast-to-void will do?
> Sure, I'll adjust it accordingly.
Feel free to improve the comment if you think it could be clearer.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> In short, this patch was ill considered. Please revert. If we need
> >> to silence a Coverity complaint, perhaps a cast-to-void will do?
>
> > Sure, I'll adjust it accordingly.
>
> Feel free to improve the comment if you think it could be clearer.
I hemmed and hawed over it and tried to improve it but I'm not convinced
that I did. Still, I went ahead and at least got the revert committed.
If anyone feels the comment change hurts more than helps, let me know.
Thanks,
Stephen