Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@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 signature.asc Description: Digital signature
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
On 01/27/2014 05:32 PM, Stephen Frost wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@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 -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
Stephen Frost sfr...@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 -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@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 -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
* Tom Lane (t...@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 signature.asc Description: Digital signature
Re: [COMMITTERS] pgsql: Check dup2() results in syslogger
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@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 -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
