[AC 21.5] Prevent SIGPIPE in deactivate_process

APPROVE COMMIT 21.5 I don't know if this is relevant to 21.4. Should I check, Vin? This seems to be a very rare crash, up until Uday Reddy and friends found me a way to produce it frequently. :-) Reviewers: If you have a clue about Unix processes, please check my analysis below, especially with respect to the call of deactivate_process() in execute_internal_event() (event-stream.c). VM seems to have replaced Gnus as my favorite way to induce XEmacs crashes.... VM uses Imagemagick's convert program to generate thumbnails to display with its MIME buttons, and apparently convert crashes a lot. When this happens, there's a race condition such that send_process() thinks it has blocked but doesn't know about the crash, and so calls Faccept_process_output() in hopes of getting the subprocess unblocked. The SIGCHLD arrives in the interim, status_notify() gets called inside of Faccept_process_output(), notices the demise of the child, and calls deactivate_process(), which tries to do a normal close of all pipes. That calls Lstream_flush(), which writes to the now nonexistent output pipe, and XEmacs takes the fall because it's not prepared for a SIGPIPE there. An analysis of calls of deactivate_process() indicates that it only gets called when we know the process has already exited, so it makes no sense to flush the output pipe. This patch removes the flush from the function closing the output pipe (only). I would like to do the signal(2) dance in the relevant Lstream implementation (filedesc), but it doesn't know about pipes and processes, only about FDs. Is it worth creating a pipe-specific variant of the filedesc Lstream? I will revert this patch on request. I don't really have an idea of what bad things could happen if we fail to flush to a live process that is about to be terminated, but if somebody thinks that's a risk, I have an alternative patch which reduces the current risk but does not eliminate it. (Specifically, with the alternative patch, the race condition still exists in Faccept_process_output(), which is a Lisp function so we have no idea when it might be called.) diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/ChangeLog --- a/src/ChangeLog Sat Oct 29 00:35:33 2011 +0900 +++ b/src/ChangeLog Sat Oct 29 01:10:32 2011 +0900 ＠＠ -1,3 +1,16 ＠＠ +2011-10-29 Stephen J. Turnbull <stephen(a)xemacs.org&gt; + + Prevent SIGPIPEs in deactivate_process(). + + * process.c (deactivate_process): + Use Lstream_close_noflush on output pipe instead of Lstream_close. + + * lstream.c (Lstream_close_noflush): + New. Factored out of Lstream_close. + (Lstream_close): Use Lstream_close_noflush. + + * lstream.h (Lstream_close_noflush): Declare it. + 2011-10-29 Stephen J. Turnbull <stephen(a)xemacs.org&gt; Prevent assert at frame.c, l. 6311 by initializing glyph cachels. diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/lstream.c --- a/src/lstream.c Sat Oct 29 00:35:33 2011 +0900 +++ b/src/lstream.c Sat Oct 29 01:10:32 2011 +0900 ＠＠ -791,6 +791,45 ＠＠ return Lstream_flush (lstr); } +/* Close the stream without flushing buffers. + In current practice, this is only useful when a subprocess terminates + unexpectedly, and the OS closes its pipes without warning. In that case, + we do not want to flush our output buffers, as there is no longer a pipe + to write to. + This nomenclature may deserve review if XEmacs starts getting called as + a subprocess. */ + +int +Lstream_close_noflush (Lstream *lstr) +{ + lstr->flags &= ~LSTREAM_FL_IS_OPEN; + lstr->byte_count = 0; + /* Note that Lstream_flush() reset all the buffer indices. That way, + the next call to Lstream_putc(), Lstream_getc(), or Lstream_ungetc() + on a closed stream will call into the function equivalents, which will + cause an error. */ + + /* We set the pointers to 0 so that we don't lose when this function + is called more than once on the same object */ + if (lstr->out_buffer) + { + xfree (lstr->out_buffer); + lstr->out_buffer = 0; + } + if (lstr->in_buffer) + { + xfree (lstr->in_buffer); + lstr->in_buffer = 0; + } + if (lstr->unget_buffer) + { + xfree (lstr->unget_buffer); + lstr->unget_buffer = 0; + } + + return 0; +} + /* Close the stream. All data will be flushed out. If the stream is already closed, nothing happens. Note that, even if all data has already been flushed out, the act of closing a stream may generate more ＠＠ -838,30 +877,7 ＠＠ rc = -1; } - lstr->flags &= ~LSTREAM_FL_IS_OPEN; - lstr->byte_count = 0; - /* Note that Lstream_flush() reset all the buffer indices. That way, - the next call to Lstream_putc(), Lstream_getc(), or Lstream_ungetc() - on a closed stream will call into the function equivalents, which will - cause an error. */ - - /* We set the pointers to 0 so that we don't lose when this function - is called more than once on the same object */ - if (lstr->out_buffer) - { - xfree (lstr->out_buffer); - lstr->out_buffer = 0; - } - if (lstr->in_buffer) - { - xfree (lstr->in_buffer); - lstr->in_buffer = 0; - } - if (lstr->unget_buffer) - { - xfree (lstr->unget_buffer); - lstr->unget_buffer = 0; - } + Lstream_close_noflush (lstr); return rc; } diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/lstream.h --- a/src/lstream.h Sat Oct 29 00:35:33 2011 +0900 +++ b/src/lstream.h Sat Oct 29 01:10:32 2011 +0900 ＠＠ -306,6 +306,7 ＠＠ int Lstream_rewind (Lstream *lstr); int Lstream_seekable_p (Lstream *lstr); int Lstream_close (Lstream *lstr); +int Lstream_close_noflush (Lstream *lstr); void Lstream_delete (Lstream *lstr); void Lstream_set_character_mode (Lstream *str); diff -r 3fde0e346ad7 -r 2dbefd79b3d3 src/process.c --- a/src/process.c Sat Oct 29 00:35:33 2011 +0900 +++ b/src/process.c Sat Oct 29 01:10:32 2011 +0900 ＠＠ -2150,8 +2150,20 ＠＠ /* Must call this before setting the streams to nil */ event_stream_unselect_process (p, 1, 1); + /* We can get here in case of a crash in the external process, and then + the Lstream_close on output will cause a SIGPIPE, which we're not ready + for here. It looks to me like all cases where this function is called + we know the process has exited (but I'm not 100% sure for the call in + execute_internal_event (event-stream.c)), so it should be OK to use + Lstream_close_noflush. + + #### The layering here needs a rethink. We should just be able + to call Lstream_close, and let the Lstream's implementation decide + if it can flush safely or not. The immediate problem is that the + Lstream needs to know the process's status, but I don't think it has + a handle to the process. */ if (!NILP (DATA_OUTSTREAM (p))) - Lstream_close (XLSTREAM (DATA_OUTSTREAM (p))); + Lstream_close_noflush (XLSTREAM (DATA_OUTSTREAM (p))); if (!NILP (DATA_INSTREAM (p))) Lstream_close (XLSTREAM (DATA_INSTREAM (p))); if (!NILP (DATA_ERRSTREAM (p))) _______________________________________________ XEmacs-Patches mailing list XEmacs-Patches(a)xemacs.org http://lists.xemacs.org/mailman/listinfo/xemacs-patches