Re: [PATCH][BZ #14782] Do not enable asynchronous cancelation in system

[OndÅej BÃlka <neleai at seznam dot cz>]

On Sun, Jan 12, 2014 at 12:37:59PM -0500, Carlos O'Donell wrote:
> On 01/12/2014 10:28 AM, Rich Felker wrote:
> > On Sun, Jan 12, 2014 at 01:13:10PM +0100, OndÅej BÃlka wrote:
> >> Hi,
> >>
> >> When looking bugs another relatively easy one is that we do not need to
> >> enable async cancellation in system.
> >>
> >> We use cancellation to kill child process and we do not need enable
> >> cancellation until we install handlers to kill a child process. A
> >> cancellation needs to be only enabled in waidpid which already does
> >> that.
> 
> This is not quite correct.
> 
> The child process won't be killed via cancellation (ignore bug 14744 for
> now) because it's another process not a thread.
> 
> We don't use any cancellation to kill the child. The parent is the one 
> which we want to cancel.
> 
> The child won't install the SIGCANCEL handler until it runs
> __pthread_initialize_minimal_internal which may be never if it's not
> MT.
> 
> You are correct in that waitpid is a cancellation point and will check
> to see if this thread was the target of a cancellation request and
> cancel the thread before the function returns.
> 
> Hopefully that clarifies things for you. If you have any questions about
> cancellation please ask them. It's a big mess right now in glibc, and
> I'm happy to share what I know.
> 
> >> OK for 2.20?
> >>
> >> 	* sysdeps/posix/system.c (__libc_system): Do not enable
> >> 	asynchronous cancellation.
> >>
> >> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c
> >> index de71e6b..e8b921f 100644
> >> --- a/sysdeps/posix/system.c
> >> +++ b/sysdeps/posix/system.c
> >> @@ -181,15 +181,6 @@ __libc_system (const char *line)
> >>         not be available after a chroot(), for example.  */
> >>      return do_system ("exit 0") == 0;
> >>  
> >> -  if (SINGLE_THREAD_P)
> >> -    return do_system (line);
> >> -
> >> -  int oldtype = LIBC_CANCEL_ASYNC ();
> >> -
> >> -  int result = do_system (line);
> >> -
> >> -  LIBC_CANCEL_RESET (oldtype);
> >> -
> >> -  return result;
> >> +  return do_system (line);
> >>  }
> >>  weak_alias (__libc_system, system)
> 
> This looks good go me... however!
> 
> OK to commit as long as you prove waitpid is actually a
> cancellation point as required by POSIX.
> 

It in list here:

http://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_09.html#tag_02_09_05_02