[PATCH 2/4] nptl: Handle EPIPE on tst-cancel2

[Florian Weimer]

* Adhemerval Zanella:

> On 20/08/2019 07:23, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> The SIGPIPE can be handled before SIGCANCEL, which makes write fail
>>> and the thread return a non expected result.
>>>
>>> Checked on x86_64-linux-gnu.
>>>
>>> 	* nptl/tst-cancel2.c (tf): Do not abort with EPIPE.
>>> ---
>>>  nptl/tst-cancel2.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/nptl/tst-cancel2.c b/nptl/tst-cancel2.c
>>> index 1f0429d343..632ea4e0ae 100644
>>> --- a/nptl/tst-cancel2.c
>>> +++ b/nptl/tst-cancel2.c
>>> @@ -20,6 +20,7 @@
>>>  #include <signal.h>
>>>  #include <stdio.h>
>>>  #include <unistd.h>
>>> +#include <errno.h>
>>>  
>>>  
>>>  static int fd[2];
>>> @@ -32,7 +33,14 @@ tf (void *arg)
>>>       write blocks.  */
>>>    char buf[100000];
>>>  
>>> -  while (write (fd[1], buf, sizeof (buf)) > 0);
>>> +  while (1)
>>> +    {
>>> +      /* Ignore EPIPE errors for case the SIGPIPE is handle before
>>> +	 SIGCANCEL.  */
>>> +      ssize_t ret = write (fd[1], buf, sizeof (buf));
>>> +      if (ret == 0 || (ret == -1 && errno != EPIPE))
>>> +       break;
>>> +    }
>>>  
>>>    return (void *) 42l;
>>>  }
>> 
>> I disagree with this change.  SIGPIPE does not appear to be a valid
>> error from write in this test because the thread is canceled before the
>> read end of the pipe is closed.  POSIX says this:
>> 
>> | The cancellation processing in the target thread shall run
>> | asynchronously with respect to the calling thread returning from
>> | pthread_cancel().
>> 
>> But that doesn't mean that the thread can observe actions in its
>> uncanceled state that happen after the cancellation, which is the case
>> when the write fails with SIGPIPE.
>
> My understanding is since thread cancellation is implemented with an 
> asynchronous signal and there is no *extra* synchronization between the
> pthread_cancel from main thread and write call on the create one, there
> is no happens before order between them. 
>
> It might the case where, due scheduling, the cancellation signal is 
> triggered on the test during the write call and it is a side effect that
> should be handled. This is exactly the kind of race conditions BZ#12683
> make more explicit.

Hmm.  What are the write call sequences for this test?

I can think of the following scenarios:

(A) The write call is canceled immediately and never returns.

(B) The write call returns a positive value.  The second write call is
    canceled.

(C) The first write call returns a positive value.  The second write
    call fails with with EPIPE.  The third write call is canceled.

I have serious doubts that that (C) is a valid sequence.  Practically
speaking, I think it will lead to regressions because threads may fail
to act upon cancellation compared to what we have today.  This works out
for the test because there is an infinite number of write calls.

Looking at the actual cancellation change, I see this:

+  /* Call the arch-specific entry points that contains the globals markers
+     to be checked by SIGCANCEL handler.  */
+  result = __syscall_cancel_arch (&pd->cancelhandling, nr, a1, a2, a3, a4, a5,
+			          a6);
+
+  if ((result == -EINTR)
+      && (pd->cancelhandling & CANCELED_BITMASK)
+      && !(pd->cancelhandling & CANCELSTATE_BITMASK))
+    __do_cancel ();

Why the restriction to EINTR?  Any signal can result in EINTR.  And with
SA_RESTART, the SIGCANCEL handler will not even result in EINTR.

Thanks,
Florian