This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/4] nptl: Handle EPIPE on tst-cancel2
On 20/08/2019 09:59, Florian Weimer wrote:
> * 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.
The idea of using a large buffer is exactly to force the write to block
indefinitely. Ideally the test should use F_SETPIPE_SZ to make it sure
it would block on first call, but current scheme might issue multiple
calls until buffer is full (which usually does not trigger because
SIGCANCEL is usually triggered beforehand).
What happens for current asynchronous handling is if the syscall is
indeed interrupted, it will call __pthread_disable_asynccancel and
then it will wait on futex_wait_simple (line 97) until the cancellation
handling is done. It waits on the futex because the pthread_cancel sets the
CANCELING_BITMASK | CANCELED_BITMASK. The sigcancel_handler then acts upon
cancellation and finishes the thread.
That's the *very* racy issue BZ#12683 aims to fix, since current cancellation
does not give the information back to program that a side-effect of the sycall
has happens (in this case EPIPE).
For tst-cancel2.c, if I add a sleep (1) between pthread_create and
pthread_cancel you can see this issue more clearly (dump with strace):
[pid 2587] set_robust_list(0x7fffabccf290, 24) = 0
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000) = 100000
[pid 2587] write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 100000 <unfinished ...>
[pid 2586] <... nanosleep resumed>0x7ffff0c9e7f0) = 0
########### Cancellation start to act here, by loading the libgcc to unwinding
[pid 2586] open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 5
[pid 2586] fstat(5, {st_mode=S_IFREG|0644, st_size=63776, ...}) = 0
[pid 2586] mmap(NULL, 63776, PROT_READ, MAP_PRIVATE, 5, 0) = 0x7fffabf00000
[pid 2586] close(5) = 0
[pid 2586] open("/lib64/libgcc_s.so.1", O_RDONLY|O_CLOEXEC) = 5
[pid 2586] read(5, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0\25\0\1\0\0\0\340+\0\0\0\0\0\0"..., 832) = 832
[pid 2586] fstat(5, {st_mode=S_IFREG|0755, st_size=133696, ...}) = 0
[pid 2586] mmap(NULL, 197688, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 5, 0) = 0x7fffab480000
[pid 2586] mmap(0x7fffab4a0000, 131072, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x10000) = 0x7fffab4a0000
[pid 2586] close(5) = 0
[pid 2586] mprotect(0x7fffab4a0000, 65536, PROT_READ) = 0
[pid 2586] munmap(0x7fffabf00000, 63776) = 0
[pid 2586] tgkill(2586, 2587, SIGRTMIN) = 0
[pid 2586] close(3) = 0
[pid 2586] futex(0x7fffabccf280, FUTEX_WAIT, 2587, NULL <unfinished ...>
########### Write returns with broken PIPE and __pthread_disable_asynccancel is called
[pid 2587] <... write resumed>) = -1 EPIPE (Broken pipe)
[pid 2587] --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=2586, si_uid=61684} ---
[pid 2587] --- SIGRTMIN {si_signo=SIGRTMIN, si_code=SI_TKILL, si_pid=2586, si_uid=61684} ---
[pid 2587] futex(0x7fffab4b0224, FUTEX_WAKE_PRIVATE, 2147483647) = 0
########### No side-effects reported back to program
[pid 2587] madvise(0x7fffab4c0000, 8257536, MADV_DONTNEED) = 0
[pid 2587] exit(0) = ?
With BZ#12683 fix the cancellation is not acted upon and the testcase then fails
depending whether the write is interrupted or not by the cancellation signal.
>
> 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.
The EINTR was a special case for __NR_close from original Rich suggestion,
which I think we will need to discuss further it is indeed make sense for
glibc (it is the long-standing issue regarding of close failure and how to
handle it). I removed it in the current version.