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

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

>> In the new implementation, if there is a signal handler and a signal for
>> it is delivered before SIGCANCEL (even if the signal was sent *after*
>> pthread_cancel, from the same thread), I do not think there is any
>> chance whatsoever that we can hide the behavior difference.  After all,
>> SIGCANCEL may not trigger an asynchronous cancellation from the signal
>> handler.
>
> Yes, although SIGCANCEL would be set as SA_RESTART, it is explicit disabled
> by the signal handle by removing it from ucontext_t signal mask.

Does this mean a thread would lose its ability to be canceled in
blocking system calls if it setjmps out of a signal handler?  (Where as
sigsetjmp would keep cancellation working.) 

>> System calls that are cancellation points appear to fall into these
>> categories:
>> 
>> (A) Those that do not perform any resource allocation (write, read).
>> 
>> (B) Those that perform resource allocation, but the allocation can be
>>     easily reverted (openat, accept4).
>> 
>> (C) Those that perform resource allocation, but the allocation is
>>     difficult to undo (recvmsg with descriptor passing).
>> 
>> (D) close.

>> For (B), maybe we should undo the resource allocation and then proceed
>> to act on the cancellation.
>
> Besides this requires a lot of more complexity by mapping what kind of
> resources each syscall would require to free and when to free based on
> returned codes, it would hide it from the application. My view we just
> need to return that the syscall has visible side-effects that may result
> in system resources leaks and let the application deal with.

My concern is that there might be no further cancellation point, and if
we do not cancel in the case of (B), we might fail to act on the
cancellation even though the canceled thread was blocked at a
cancellation point when pthread_cancel was called.

>> For (C), the complexity may not be worth it.
>> 
>> For (A), (B), (C), we can act on the cancellation in the error case,
>> after we observe the cancellation flag in the signal handler trampoline.
>> (I think dropping the EINTR restriction from there achieves that.)  If
>> we do that, we do not need to change the test case.
>
> I don't see a real net gain in adding such complexity.  It is not on the 
> error  case, but rather when visible side-effects has happened and we should
> alert the program to act upon that. The tst-cancel2 case is an error case
> because of the EPIPE semantic, but the new tst-cancel28 (based on the leak
> example from BZ#12683) is a case where the open call returns a valid file
> descriptor that should be freed.

I totally agree that the descriptor must not leak.  However,
tst-cancel28 is written in such a way that it does not expect an error
from open:

static void *
leaker (void *arg)
{
  int fd = open (arg, O_RDONLY);
  pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0);
  close (fd);
  return NULL;
}

It would be clearer if it actually used error checking, but we call
close (-1) on error, which is invalid.

If this test represents how application code is actually written, I
think we really need to check for cancellation before returning the file
descriptor.

> If glibc starts to add extra semantics for cancellation, we will have
> a standard extensions since now 'open', for instance, would close the
> file descriptor if the cancellation signal is delivered after the OS
> allocates the file descriptor resource.  IMHO this is clearly what the
> program itself should be handling, because it might the case where it
> is prepared to close itself the file descriptor in a different thread.

open is a cancellation point, so according to POSIX, the thread is
canceled if the open call completes after pthread_cancel returns.  (Due
to integration with the memory model, thread ordering issues are
difficult to reason about in POSIX, though.)  I do not think we should
return a descriptor to the application that can only exist because of
something that happened *after* the pthread_cancel call.

> This would lead to possible another incompatibilities regarding libc
> system implementation, besides adding more complexity.

I think we would just implement POSIX behavior more closely because open
is required to be a cancellation point.  It's unfortunate that we do not
have kernel support for this.  Other systems can likely implement
cancellation semantics correctly more easily.

>> (D) is very special.  Ideally, we would specify what happens with the
>> descriptor if the close call is canceled.  POSIX does not even specify
>> what the state of file descriptor is after an EINTR error, so it doesn't
>> say what happens with cancellation, either.  Maybe we have to leave that
>> undefined.
>
> Indeed even after the POSIX close language clarification [1], the specification 
> is messy (it really does not help that some system does have interruptible
> close implementation that might return -1).
>
> However my understanding is we can assume POSIX_CLOSE_RESTART 0 for Linux
> since it shouldn't return -1/EINTR in any case (and I don't know if/when
> there are kernels that actually does it) [2].  So assuming the nptl is Linux 
> specific, we can assume that, although close is specified as a cancellable
> entrypoint, it won't happen in practice.
>
> [1] http://austingroupbugs.net/view.php?id=529
> [2] https://lwn.net/Articles/576478/

I think this is slightly different from the EINTR case.  With
SA_RESTART, perhaps the signal handler can run *before* the descriptor
is closed, so if we never return from the handler, the descriptor leaks?

Thanks,
Florian