Re: [PATCH] libc: make system() block SIGCHLD
On Wed, Jan 11, 2012 at 06:37:31PM +0100, Richard Braun wrote: > When built with an old thread implementation (or for a sparc target), > the implementation of the system() function doesn't conform to its > specification. Namely, it resets the SIGCHLD handler to its default > instead of blocking the signal, which may result in "lost" signals > if a custom handler was installed. Replace this by appropriate calls > to sigprocmask(). sigprocmask alone is not sufficient to implement the required behavior for system. In particular, in a multi-threaded program, another thread will receive the signal and reap the child, causing system to malfunction. Really there's no safe way to use system in a multi-threaded program (or whatsoever, IMO), but to conform to POSIX it must block SIGCHLD: The system() function shall ignore the SIGINT and SIGQUIT signals, and shall block the SIGCHLD signal, while waiting for the command to terminate. If this might cause the application to miss a signal that would have killed it, then the application should examine the return value from system() and take whatever action is appropriate to the application if the command terminated due to receipt of a signal. Source: http://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Fri, Jan 13, 2012 at 10:09:04PM -0500, Rich Felker wrote: > sigprocmask alone is not sufficient to implement the required behavior > for system. In particular, in a multi-threaded program, another thread > will receive the signal and reap the child, causing system to > malfunction. Really there's no safe way to use system in a > multi-threaded program (or whatsoever, IMO), but to conform to POSIX > it must block SIGCHLD: Agreed. This is why I didn't use pthread_sigmask(). I mention Linuxthreads because there are two variants of the system() function in uClibc, and the other one, which depends on defined __UCLIBC_HAS_THREADS_NATIVE__ && !defined __sparc__, isn't concerned by this issue. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Friday 13 January 2012 22:09:04 Rich Felker wrote: > In particular, in a multi-threaded program also from that POSIX page: * The system() function need not be thread-safe. makes me wonder if we shouldn't just gut the pthread locking from the NPTL version that was straight copied from glibc -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Wednesday 11 January 2012 12:37:31 Richard Braun wrote: > When built with an old thread implementation (or for a sparc target), > the implementation of the system() function doesn't conform to its > specification. Namely, it resets the SIGCHLD handler to its default > instead of blocking the signal, which may result in "lost" signals > if a custom handler was installed. Replace this by appropriate calls > to sigprocmask(). if you unify the error paths with goto's, do you get smaller code size ? -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sat, Jan 14, 2012 at 06:33:55AM -0500, Mike Frysinger wrote: > if you unify the error paths with goto's, do you get smaller code size ? Probably. I tried to follow the style of other code pieces I've read, which is why I avoided using gotos in the first place. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sat, Jan 14, 2012 at 02:03:45PM +0100, Richard Braun wrote: > + if (sigprocmask(SIG_SETMASK, &omask, NULL) != 0) > + ret_val = -1; If you consider there is no need to check the return value of sigprocmask() (which should only fail if parameters are invalid AIUI), feel free to adjust that part. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Saturday 14 January 2012 07:39:11 Richard Braun wrote: > On Sat, Jan 14, 2012 at 06:33:55AM -0500, Mike Frysinger wrote: > > if you unify the error paths with goto's, do you get smaller code size ? > > Probably. I tried to follow the style of other code pieces I've read, > which is why I avoided using gotos in the first place. we prefer small wherever possible :) orig patch: 502 0 0 502 1f6 libc/stdlib/system.os goto version: 432 0 0 432 1b0 libc/stdlib/system.oS -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Saturday 14 January 2012 08:21:27 Richard Braun wrote: > On Sat, Jan 14, 2012 at 02:03:45PM +0100, Richard Braun wrote: > > + if (sigprocmask(SIG_SETMASK, &omask, NULL) != 0) > > + ret_val = -1; > > If you consider there is no need to check the return value of > sigprocmask() (which should only fail if parameters are invalid AIUI), > feel free to adjust that part. i am inclined in dropping these checks since there's no realistic way they could fail (where something else wasn't broken already). let's see if anyone else has an opinion ... before your patch (x86_64): 417 0 0 417 1a1 libc/stdlib/system.os current goto one: 432 0 0 432 1b0 libc/stdlib/system.os no error checking: 412 0 0 412 19c libc/stdlib/system.os not only did you fix standards compliance, but you managed to shrink it at the same time. good on you! -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sun, Jan 15, 2012 at 12:46:33PM +0100, Richard Braun wrote: > When built without NPTL support (or for a sparc target), the system() > function doesn't conform to its specification. Namely, it resets the > SIGCHLD handler to its default instead of blocking the signal, which > may result in "lost" signals if a custom handler was installed. > Replace this by appropriate calls to sigprocmask(). Your report is wrong. system is REQUIRED by POSIX to change the signal disposition for SIGCHLD, not just to block the signal. It should change it to SIG_IGN, not SIG_DFL, but for practical purposes these are the same or similar. The code you sent the patch for is buggy, however, since it uses signal() rather than sigaction() and thus cannot correctly restore the signal disposition if it was installed with sigaction(). Rich ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sun, Jan 15, 2012 at 10:04:58AM -0500, Rich Felker wrote: > Your report is wrong. system is REQUIRED by POSIX to change the signal > disposition for SIGCHLD, not just to block the signal. It should > change it to SIG_IGN, not SIG_DFL, but for practical purposes these > are the same or similar. I didn't notice that anywhere. Could you indicate where this behaviour is specified exactly ? > The code you sent the patch for is buggy, > however, since it uses signal() rather than sigaction() and thus > cannot correctly restore the signal disposition if it was installed > with sigaction(). I agree the use of signal() immediately disturbed me, but I didn't read its implementation. Maybe it does the job. In any case, we could use the occasion to fix that as well and replace signal() with sigaction() in the same patch. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sun, Jan 15, 2012 at 08:36:55PM +0100, Richard Braun wrote: > I agree the use of signal() immediately disturbed me, but I didn't read > its implementation. Maybe it does the job. In any case, we could use the > occasion to fix that as well and replace signal() with sigaction() in > the same patch. In addition, I wonder if the code in the parent immediately following vfork(), which forces SIGINT and SIGQUIT to SIG_IGN once more, is really useful. AIUI, this system call should have no effect on the parent signal dispositions. I'll wait for more feedback before submitting a new patch. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sunday 15 January 2012 14:36:55 Richard Braun wrote: > On Sun, Jan 15, 2012 at 10:04:58AM -0500, Rich Felker wrote: > > Your report is wrong. system is REQUIRED by POSIX to change the signal > > disposition for SIGCHLD, not just to block the signal. It should > > change it to SIG_IGN, not SIG_DFL, but for practical purposes these > > are the same or similar. > > I didn't notice that anywhere. Could you indicate where this behaviour > is specified exactly ? http://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html The system() function shall ignore the SIGINT and SIGQUIT signals, and shall block the SIGCHLD signal, while waiting for the command to terminate > > The code you sent the patch for is buggy, > > however, since it uses signal() rather than sigaction() and thus > > cannot correctly restore the signal disposition if it was installed > > with sigaction(). > > I agree the use of signal() immediately disturbed me, but I didn't read > its implementation. Maybe it does the job. In any case, we could use the > occasion to fix that as well and replace signal() with sigaction() in > the same patch. Rich is referring to the extended aspects of signal handling that sigaction() enables over signal() such as sa_sigaction and sa_flags. signal() will save/restore the handler just fine, but otherwise this function does subtly break a few things ... if you're up for fixing the code beyond what you've already done, that'd be cool :) -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sunday 15 January 2012 14:50:24 Richard Braun wrote: > On Sun, Jan 15, 2012 at 08:36:55PM +0100, Richard Braun wrote: > > I agree the use of signal() immediately disturbed me, but I didn't read > > its implementation. Maybe it does the job. In any case, we could use the > > occasion to fix that as well and replace signal() with sigaction() in > > the same patch. > > In addition, I wonder if the code in the parent immediately following > vfork(), which forces SIGINT and SIGQUIT to SIG_IGN once more, is really > useful. AIUI, this system call should have no effect on the parent > signal dispositions. i think we can go ahead and rely on the real world at this point and say that the vfork() call did not screw with our signal() setup. i.e. go ahead and delete those two lines. -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Sun, Jan 15, 2012 at 06:51:15PM -0500, Mike Frysinger wrote: > On Sunday 15 January 2012 14:36:55 Richard Braun wrote: > > On Sun, Jan 15, 2012 at 10:04:58AM -0500, Rich Felker wrote: > > > Your report is wrong. system is REQUIRED by POSIX to change the signal > > > disposition for SIGCHLD, not just to block the signal. It should > > > change it to SIG_IGN, not SIG_DFL, but for practical purposes these > > > are the same or similar. > > > > I didn't notice that anywhere. Could you indicate where this behaviour > > is specified exactly ? > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html > The system() function shall ignore the SIGINT and SIGQUIT signals, and > shall block the SIGCHLD signal, while waiting for the command to > terminate That part doesn't state "system is REQUIRED by POSIX to change the signal disposition for SIGCHLD, not just to block the signal", on the contrary, which is why I'm asking. > > I agree the use of signal() immediately disturbed me, but I didn't read > > its implementation. Maybe it does the job. In any case, we could use the > > occasion to fix that as well and replace signal() with sigaction() in > > the same patch. > > Rich is referring to the extended aspects of signal handling that sigaction() > enables over signal() such as sa_sigaction and sa_flags. signal() will > save/restore the handler just fine, but otherwise this function does subtly > break a few things ... Sure, but I was implying the implementation of signal() might have used some trick to correctly handle such cases, which is highly unlikely. -- Richard Braun ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] libc: make system() block SIGCHLD
On Monday 16 January 2012 04:00:08 Richard Braun wrote: > On Sun, Jan 15, 2012 at 06:51:15PM -0500, Mike Frysinger wrote: > > On Sunday 15 January 2012 14:36:55 Richard Braun wrote: > > > On Sun, Jan 15, 2012 at 10:04:58AM -0500, Rich Felker wrote: > > > > Your report is wrong. system is REQUIRED by POSIX to change the > > > > signal disposition for SIGCHLD, not just to block the signal. It > > > > should change it to SIG_IGN, not SIG_DFL, but for practical purposes > > > > these are the same or similar. > > > > > > I didn't notice that anywhere. Could you indicate where this behaviour > > > is specified exactly ? > > > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html > > > > The system() function shall ignore the SIGINT and SIGQUIT signals, and > > shall block the SIGCHLD signal, while waiting for the command to > > terminate > > That part doesn't state "system is REQUIRED by POSIX to change the > signal disposition for SIGCHLD, not just to block the signal", on the > contrary, which is why I'm asking. you're right ... i don't see POSIX requiring the signal disposition being reset. Rich will have to explain why he thinks it does. the POSIX example at the end of the system() page doesn't do that either. > > > I agree the use of signal() immediately disturbed me, but I didn't read > > > its implementation. Maybe it does the job. In any case, we could use > > > the occasion to fix that as well and replace signal() with sigaction() > > > in the same patch. > > > > Rich is referring to the extended aspects of signal handling that > > sigaction() enables over signal() such as sa_sigaction and sa_flags. > > signal() will save/restore the handler just fine, but otherwise this > > function does subtly break a few things ... > > Sure, but I was implying the implementation of signal() might have used > some trick to correctly handle such cases, which is highly unlikely. the only trick signal() itself does is change its default behavior based on the feature macros in use. see the end of the signal(2) man page (portability section) where it describes the behavior under Linux. there's no sane way for it to save/restore the full signal handling state since the return value is a single pointer. -mike signature.asc Description: This is a digitally signed message part. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
