comments?
alternatives: sigsetjmp(ugly) and pselect(not portable, available)
drawback: additional filedescriptors.
Index: serverloop.c
==================================================================RCS file:
/home/markus/cvs/ssh/serverloop.c,v
retrieving revision 1.82
diff -u -r1.82 serverloop.c
--- serverloop.c 10 Oct 2001 22:18:47 -0000 1.82
+++ serverloop.c 11 Oct 2001 18:06:33 -0000
@@ -92,6 +92,45 @@
/* prototypes */
static void server_init_dispatch(void);
+/*
+ * we write to this pipe if a SIGCHLD is caught in order to avoid
+ * the race between select() and child_terminated
+ */
+static int notify_pipe[2];
+static void
+notify_setup(void)
+{
+ if (pipe(notify_pipe) < 0) {
+ error("pipe(notify_pipe) failed %s", strerror(errno));
+ notify_pipe[0] = -1; /* read end */
+ notify_pipe[1] = -1; /* write end */
+ } else {
+ set_nonblock(notify_pipe[0]);
+ set_nonblock(notify_pipe[1]);
+ }
+}
+static void
+notify_parent(void)
+{
+ if (notify_pipe[1] != -1)
+ write(notify_pipe[1], "", 1);
+}
+static void
+notify_prepare(fd_set *readset)
+{
+ if (notify_pipe[0] != -1)
+ FD_SET(notify_pipe[0], readset);
+}
+static void
+notify_done(fd_set *readset)
+{
+ char c;
+
+ if (notify_pipe[0] != -1 && FD_ISSET(notify_pipe[0], readset))
+ while (read(notify_pipe[0], &c, 1) != -1)
+ debug2("notify_done: reading");
+}
+
static void
sigchld_handler(int sig)
{
@@ -99,6 +138,7 @@
debug("Received SIGCHLD.");
child_terminated = 1;
signal(SIGCHLD, sigchld_handler);
+ notify_parent();
errno = save_errno;
}
@@ -242,6 +282,7 @@
if (fdin != -1 && buffer_len(&stdin_buffer) > 0)
FD_SET(fdin, *writesetp);
}
+ notify_prepare(*readsetp);
/*
* If we have buffered packet data going to the client, mark that
@@ -278,6 +319,8 @@
error("select: %.100s", strerror(errno));
} else if (ret == 0 && client_alive_scheduled)
client_alive_check();
+
+ notify_done(*readsetp);
}
/*
@@ -467,6 +510,8 @@
connection_in = packet_get_connection_in();
connection_out = packet_get_connection_out();
+ notify_setup();
+
previous_stdout_buffer_bytes = 0;
/* Set approximate I/O buffer size. */
@@ -572,6 +617,7 @@
max_fd = MAX(max_fd, fdin);
max_fd = MAX(max_fd, fdout);
max_fd = MAX(max_fd, fderr);
+ max_fd = MAX(max_fd, notify_pipe[0]);
/* Sleep in select() until we can do something. */
wait_until_can_do_something(&readset, &writeset, &max_fd,
@@ -696,7 +742,11 @@
connection_in = packet_get_connection_in();
connection_out = packet_get_connection_out();
+ notify_setup();
+
max_fd = MAX(connection_in, connection_out);
+ max_fd = MAX(max_fd, notify_pipe[0]);
+
xxx_authctxt = authctxt;
server_init_dispatch();
Does this make it unnecessary to block SIGCHLD around where child_terminated is manipulated? At first glance I'd say yes... Your patch is rather elegant and simple. The extra file descriptors needed should not be a problem, I don't think. Why did you list that as a disadvantage? Below is a patch to misc.h/misc.c/config.h.in to deal with signal blocking. And sigprocmask and sigblock need to be added to the configure.in list of function checks. And the code in serverloop2() that manipulates child_terminated needs to be bracketed with calls to block_sigchld()/unblock_sigchld(). I'm not recommending this patch, mind you. I'll test yours... Nico On Wed, Oct 31, 2001 at 05:42:01PM +0100, Markus Friedl wrote:> comments? > > alternatives: sigsetjmp(ugly) and pselect(not portable, available) > drawback: additional filedescriptors. >Index: 2_9_p2_w_gss_krb5_named_keys.15/config.h.in --- 2_9_p2_w_gss_krb5_named_keys.15/config.h.in Tue, 26 Jun 2001 16:27:13 -0400 (OpenSSH/f/45_config.h.i 1.3 644) +++ 2_9_p2_w_gss_krb5_named_keys.16(w)/config.h.in Tue, 30 Oct 2001 15:55:15 -0500 (OpenSSH/f/45_config.h.i 1.4 644) @@ -542,6 +542,12 @@ /* Define if you have the sigvec function. */ #undef HAVE_SIGVEC +/* Define if you have the sigprocmask function. */ +#undef HAVE_SIGPROCMASK + +/* Define if you have the sigblock function. */ +#undef HAVE_SIGBLOCK + /* Define if you have the snprintf function. */ #undef HAVE_SNPRINTF Index: 2_9_p2_w_gss_krb5_named_keys.15/misc.h --- 2_9_p2_w_gss_krb5_named_keys.15/misc.h Thu, 03 May 2001 16:12:13 -0400 (OpenSSH/i/45_misc.h 1.1 644) +++ 2_9_p2_w_gss_krb5_named_keys.16(w)/misc.h Tue, 30 Oct 2001 14:33:44 -0500 (OpenSSH/i/45_misc.h 1.2 644) @@ -32,3 +32,6 @@ /* wrapper for signal interface */ typedef void (*mysig_t)(int); mysig_t mysignal(int sig, mysig_t act); + +void block_sigchld(); +void unblock_sigchld(); Index: 2_9_p2_w_gss_krb5_named_keys.15/misc.c --- 2_9_p2_w_gss_krb5_named_keys.15/misc.c Fri, 22 Jun 2001 15:59:54 -0400 (OpenSSH/i/46_misc.c 1.2 644) +++ 2_9_p2_w_gss_krb5_named_keys.16(w)/misc.c Tue, 30 Oct 2001 16:22:43 -0500 (OpenSSH/i/46_misc.c 1.3 644) @@ -156,3 +156,34 @@ return (signal(sig, act)); #endif } + +#ifdef HAVE_SIGPROCMASK +static sigset_t osigset; +#elif HAVE_SIGBLOCK +static int osigmask; +#endif + +void +block_sigchld() +{ +#ifdef HAVE_SIGPROCMASK + sigset_t sigchld_set; + (void) sigemptyset(&sigchld_set); + (void) sigaddset(&sigchld_set, SIGCHLD); + if (sigprocmask(SIG_BLOCK, &sigchld_set, &osigset) < 0) + debug("block_sigchld: sigprocmask() returned error %s", strerror(errno)); +#elif HAVE_SIGBLOCK + osigmask = sigblock(sigmask(SIGCHLD)); +#endif +} + +void +unblock_sigchld() +{ +#ifdef HAVE_SIGPROCMASK + if (sigprocmask(SIG_UNBLOCK, &osigset, NULL) < 0) + debug("block_sigchld: sigprocmask() returned error %s", strerror(errno)); +#elif HAVE_SIGBLOCK + (void) sigsetmask(osigmask); +#endif +} Visit our website at http://www.ubswarburg.com This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message which arise as a result of e-mail transmission. If verification is required please request a hard-copy version. This message is provided for informational purposes and should not be construed as a solicitation or offer to buy or sell any securities or related financial instruments.
On Wed, Oct 31, 2001 at 05:42:01PM +0100, Markus Friedl wrote:> comments?Works. The SIGCHLD/select() race is still present in 3.0.2p1. This patch should be applied. The patch applies cleanly but for the bit that patches the signal handler - the change from signal(...) to mysignal() confuses patch.> alternatives: sigsetjmp(ugly) and pselect(not portable, available) > drawback: additional filedescriptors.sigsetjmp() is not ugly (this is the sort of problem it's meant to be there for). But it's not as elegant as the pipe trick and probably much less portable. I prefer the the pipe trick. I've never used pselect(). So I second the use of the pipe trick to prevent the SIGCHLD/select() race condition. I'll file a bug report. Thanks! Cheers, Nico> Index: serverloop.c > ==================================================================> RCS file: /home/markus/cvs/ssh/serverloop.c,v > retrieving revision 1.82 > diff -u -r1.82 serverloop.c > --- serverloop.c 10 Oct 2001 22:18:47 -0000 1.82 > +++ serverloop.c 11 Oct 2001 18:06:33 -0000 > @@ -92,6 +92,45 @@ > /* prototypes */ > static void server_init_dispatch(void); > > +/* > + * we write to this pipe if a SIGCHLD is caught in order to avoid > + * the race between select() and child_terminated > + */ > +static int notify_pipe[2]; > +static void > +notify_setup(void) > +{ > + if (pipe(notify_pipe) < 0) { > + error("pipe(notify_pipe) failed %s", strerror(errno)); > + notify_pipe[0] = -1; /* read end */ > + notify_pipe[1] = -1; /* write end */ > + } else { > + set_nonblock(notify_pipe[0]); > + set_nonblock(notify_pipe[1]); > + } > +} > +static void > +notify_parent(void) > +{ > + if (notify_pipe[1] != -1) > + write(notify_pipe[1], "", 1); > +} > +static void > +notify_prepare(fd_set *readset) > +{ > + if (notify_pipe[0] != -1) > + FD_SET(notify_pipe[0], readset); > +} > +static void > +notify_done(fd_set *readset) > +{ > + char c; > + > + if (notify_pipe[0] != -1 && FD_ISSET(notify_pipe[0], readset)) > + while (read(notify_pipe[0], &c, 1) != -1) > + debug2("notify_done: reading"); > +} > + > static void > sigchld_handler(int sig) > { > @@ -99,6 +138,7 @@ > debug("Received SIGCHLD."); > child_terminated = 1; > signal(SIGCHLD, sigchld_handler); > + notify_parent(); > errno = save_errno; > } > > @@ -242,6 +282,7 @@ > if (fdin != -1 && buffer_len(&stdin_buffer) > 0) > FD_SET(fdin, *writesetp); > } > + notify_prepare(*readsetp); > > /* > * If we have buffered packet data going to the client, mark that > @@ -278,6 +319,8 @@ > error("select: %.100s", strerror(errno)); > } else if (ret == 0 && client_alive_scheduled) > client_alive_check(); > + > + notify_done(*readsetp); > } > > /* > @@ -467,6 +510,8 @@ > connection_in = packet_get_connection_in(); > connection_out = packet_get_connection_out(); > > + notify_setup(); > + > previous_stdout_buffer_bytes = 0; > > /* Set approximate I/O buffer size. */ > @@ -572,6 +617,7 @@ > max_fd = MAX(max_fd, fdin); > max_fd = MAX(max_fd, fdout); > max_fd = MAX(max_fd, fderr); > + max_fd = MAX(max_fd, notify_pipe[0]); > > /* Sleep in select() until we can do something. */ > wait_until_can_do_something(&readset, &writeset, &max_fd, > @@ -696,7 +742,11 @@ > connection_in = packet_get_connection_in(); > connection_out = packet_get_connection_out(); > > + notify_setup(); > + > max_fd = MAX(connection_in, connection_out); > + max_fd = MAX(max_fd, notify_pipe[0]); > + > xxx_authctxt = authctxt; > > server_init_dispatch();-- -DISCLAIMER: an automatically appended disclaimer may follow. By posting- -to a public e-mail mailing list I hereby grant permission to distribute- -and copy this message.- Visit our website at http://www.ubswarburg.com This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message which arise as a result of e-mail transmission. If verification is required please request a hard-copy version. This message is provided for informational purposes and should not be construed as a solicitation or offer to buy or sell any securities or related financial instruments.
Maybe Matching Threads
- [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).
- [libguestfs PATCH] lib: remove guestfs_int_cmd_clear_close_files()
- openssh 2.9p1: data loss when stdout sent to a pipe
- Multiple (multiplexed) simultaneous ssh connections - Cygwin bug?
- SIGCHLD race *trivial* patch