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

["Carlos O'Donell" <carlos at redhat dot com>]

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.

> In any case I'd be happy to see you commit your version.

I agree with Rich this needed fixing.

Calls to sigaction, sigprocmask, fork/clone, and execve, all
of which are called before waitpid, should not block. If they
block then the kernel is broken and cancellation is not your
problem.

The cancellation point in waitpid should take care of everything.

I'm worried that Ulrich's commit 6ee8d334 was fixing something
important that we don't know about, but without a policy of
creating public BZs to track the user visible defect we can't
know what was intended to be fixed. It looks like a simply
papering over of the problem.

Summarize:
- If you can show waitpid in glibc is a cancellation point then
  you are free to checkin this fix.

Cheers,
Carlos.