| Message ID | 20190815211843.22799-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] Add RTLD_SINGLE_THREAD_P on generic single-thread.h | expand |
I will push shortly if no one opposes it. On 15/08/2019 18:18, Adhemerval Zanella wrote: > 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; > } >
* 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. Thanks, Florian
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.
* 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
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.
* Adhemerval Zanella: > 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. Hmm. Which cancellation implementation is this? At which point in the trace do we start unwinding? I'm surprised that strace reports the EPIPE before the SIGPIPE, but maybe that's just a kernel race. My expectation is that the current code unwinds after the system call returns with the EPIPE error, never returning it to the application. I think this is the right behavior for the write system call. Thanks, Florian
On 20/08/2019 12:30, Florian Weimer wrote: > * Adhemerval Zanella: > >> 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. > > Hmm. Which cancellation implementation is this? At which point in the > trace do we start unwinding? I'm surprised that strace reports the > EPIPE before the SIGPIPE, but maybe that's just a kernel race. My > expectation is that the current code unwinds after the system call > returns with the EPIPE error, never returning it to the application. I > think this is the right behavior for the write system call. This is current implement, more specifically glibc 2.17, CentOS 7.6 on powerpc64le. From the trace : >> [pid 2586] tgkill(2586, 2587, SIGRTMIN) = 0 This where pthread_cancel sends the SIGCANCEL signal to thread. >> [pid 2586] close(3) = 0 This is the /* This will cause the write in the child to return. */ close (fd[0]); In tst-cancel2.c. And finally: >> [pid 2587] <... write resumed>) = -1 EPIPE (Broken pipe) >> [pid 2587] --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=2586, si_uid=61684} --- SIGPIPE is receives, making the write fail with EPIPE and then >> [pid 2587] --- SIGRTMIN {si_signo=SIGRTMIN, si_code=SI_TKILL, si_pid=2586, si_uid=61684} --- sigcancel_handler is issued. And the implementation *does* unwind after the syscall is done, the problem is it ignores -1/EPIPE (and it is BZ#12683).
On 20/08/2019 13:46, Adhemerval Zanella wrote: > > > On 20/08/2019 12:30, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> 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. >> >> Hmm. Which cancellation implementation is this? At which point in the >> trace do we start unwinding? I'm surprised that strace reports the >> EPIPE before the SIGPIPE, but maybe that's just a kernel race. My >> expectation is that the current code unwinds after the system call >> returns with the EPIPE error, never returning it to the application. I >> think this is the right behavior for the write system call. > > This is current implement, more specifically glibc 2.17, CentOS 7.6 on > powerpc64le. From the trace : > >>> [pid 2586] tgkill(2586, 2587, SIGRTMIN) = 0 > > This where pthread_cancel sends the SIGCANCEL signal to thread. > >>> [pid 2586] close(3) = 0 > > This is the > > /* This will cause the write in the child to return. */ > close (fd[0]); > > In tst-cancel2.c. > > And finally: > >>> [pid 2587] <... write resumed>) = -1 EPIPE (Broken pipe) >>> [pid 2587] --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=2586, si_uid=61684} --- > > SIGPIPE is receives, making the write fail with EPIPE and then > >>> [pid 2587] --- SIGRTMIN {si_signo=SIGRTMIN, si_code=SI_TKILL, si_pid=2586, si_uid=61684} --- > > sigcancel_handler is issued. And the implementation *does* unwind after the > syscall is done, the problem is it ignores -1/EPIPE (and it is BZ#12683). > Florian, do still disagree with this change?
* Adhemerval Zanella:
> Florian, do still disagree with this change?
Sorry, I need to run more experiments, hopefully today.
Thanks,
Florian
* Adhemerval Zanella: > On 20/08/2019 12:30, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> 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. >> >> Hmm. Which cancellation implementation is this? At which point in the >> trace do we start unwinding? I'm surprised that strace reports the >> EPIPE before the SIGPIPE, but maybe that's just a kernel race. My >> expectation is that the current code unwinds after the system call >> returns with the EPIPE error, never returning it to the application. I >> think this is the right behavior for the write system call. > > This is current implement, more specifically glibc 2.17, CentOS 7.6 on > powerpc64le. From the trace : > >>> [pid 2586] tgkill(2586, 2587, SIGRTMIN) = 0 > > This where pthread_cancel sends the SIGCANCEL signal to thread. > >>> [pid 2586] close(3) = 0 > > This is the > > /* This will cause the write in the child to return. */ > close (fd[0]); > > In tst-cancel2.c. > > And finally: > >>> [pid 2587] <... write resumed>) = -1 EPIPE (Broken pipe) >>> [pid 2587] --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=2586, si_uid=61684} --- > > SIGPIPE is receives, making the write fail with EPIPE and then > >>> [pid 2587] --- SIGRTMIN {si_signo=SIGRTMIN, si_code=SI_TKILL, si_pid=2586, si_uid=61684} --- > > sigcancel_handler is issued. And the implementation *does* unwind after the > syscall is done, the problem is it ignores -1/EPIPE (and it is BZ#12683). I looked at this some more. It seems that in theory, we could also see EPIPE in the old implementation or at least observe execution of a signal handler, and the asynchronous cancel hides that. 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. 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 (A), I think POSIX allows sufficient wiggle room so that we can exhibit a partial side effect and still act on the cancellation (without reporting a partial write first to the application). For (B), maybe we should undo the resource allocation and then proceed to act on the cancellation. 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. (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. Thanks, Florian
On 23/08/2019 09:47, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 20/08/2019 12:30, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> 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. >>> >>> Hmm. Which cancellation implementation is this? At which point in the >>> trace do we start unwinding? I'm surprised that strace reports the >>> EPIPE before the SIGPIPE, but maybe that's just a kernel race. My >>> expectation is that the current code unwinds after the system call >>> returns with the EPIPE error, never returning it to the application. I >>> think this is the right behavior for the write system call. >> >> This is current implement, more specifically glibc 2.17, CentOS 7.6 on >> powerpc64le. From the trace : >> >>>> [pid 2586] tgkill(2586, 2587, SIGRTMIN) = 0 >> >> This where pthread_cancel sends the SIGCANCEL signal to thread. >> >>>> [pid 2586] close(3) = 0 >> >> This is the >> >> /* This will cause the write in the child to return. */ >> close (fd[0]); >> >> In tst-cancel2.c. >> >> And finally: >> >>>> [pid 2587] <... write resumed>) = -1 EPIPE (Broken pipe) >>>> [pid 2587] --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=2586, si_uid=61684} --- >> >> SIGPIPE is receives, making the write fail with EPIPE and then >> >>>> [pid 2587] --- SIGRTMIN {si_signo=SIGRTMIN, si_code=SI_TKILL, si_pid=2586, si_uid=61684} --- >> >> sigcancel_handler is issued. And the implementation *does* unwind after the >> syscall is done, the problem is it ignores -1/EPIPE (and it is BZ#12683). > > I looked at this some more. It seems that in theory, we could also see > EPIPE in the old implementation or at least observe execution of a > signal handler, and the asynchronous cancel hides that. > > 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. > > 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 (A), I think POSIX allows sufficient wiggle room so that we can > exhibit a partial side effect and still act on the cancellation (without > reporting a partial write first to the application). I agree, the side-effects for such syscalls should not lead to possible resources leakage. > > 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. > > 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. 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. This would lead to possible another incompatibilities regarding libc system implementation, besides adding more complexity. I would like to make cancellation racy-free with minimal semantic change as possible. > > (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/
* 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
On 26/08/2019 07:06, Florian Weimer wrote: > * 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.) The setjmp does not really matter here, once the async-cancellation fails to act due side-effects SIGCANCEL is removed from thread signal mask. The idea of making it sticky is always allow the thread to act on such cases regardless whether it calls another cancellation entrypoint. The thread will still be cancelled in the next cancellation entrypoint due it is marked as CANCELED_BIT. The __syscall_cancel_arch will check for the bit and call __do_cancel before issuing the syscall. > >>> 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. I shared your concern, but my understanding of the whole BZ#12683 issue is we can't really cancel the thread for the (B) case. > >>> 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. We can hardness the test to check for open failures (due file descriptor exhaustion or other arcane issue), but the point of the test is fd is always for the *close* call. If the open syscall is interrupted before the file descriptor is actually opened, the thread will be cancelled before close call. Otherwise, the kernel will have to return a valid value and indicate on the PC from the ucontext_t that the syscall has side-effect. > > 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. That's not the interpretation I see, the cancellation description for such cases is afaiu: "The side-effects of acting upon a cancellation request while suspended during a call of a function are the same as the side-effects that may be seen in a single-threaded program when a call to a function is interrupted by a signal and the given function returns [EINTR]" And on signal concepts: "[EINTR] Interrupted function call. An asynchronous signal was caught by the process during the execution of an interruptible function. If the signal handler performs a normal return, the interrupted function call may return this condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)." And my understanding is, since glibc implemented cancellation using signals and it requires to use SA_RESTART we can actually return to user if the syscall has side-effects visible to caller. It follows the same interpretation Rich used in explaining the semantic required for the a possible kernel helper for cancellation [1] > >> 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. I don't think there is an easy solution that kernel can provide to help cancellation to be standard conformant *and* race free. On the same kernel thread [2], Rich discuss some option with kernel developers and although the conclusion is open the ideas are either no conformant (the 'stick signal') or feasible (a new set of syscalls with an extra sigmak mask). > >>> (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? My understanding is the cancellation handler for close won't interrupt the syscall, regardless SA_RESTART. But indeed it might be the case where the signal is acted just before the syscall is issue and after the test for CANCELED_BITMASK is done. In this case the thread will be cancelled and the descriptor leaked. The musl handler SYS_close differently by not using the syscall bridge, so cancellation can't be acted upon and so close always succeed. On the tst-cancel28.c, pthread cancellation is explicit disable before calling close. [1] https://lkml.org/lkml/2016/3/9/1065 [2] https://lkml.org/lkml/2016/3/10/424
* Adhemerval Zanella: > On 26/08/2019 07:06, Florian Weimer wrote: >> * 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.) > The setjmp does not really matter here, once the async-cancellation fails > to act due side-effects SIGCANCEL is removed from thread signal mask. The > idea of making it sticky is always allow the thread to act on such cases > regardless whether it calls another cancellation entrypoint. I see. But why are we doing this? Can't we suppress the cancel signal inside pthread_cancel. Then we don't have to worry about the location of the signal mask in the signal context. > The thread will still be cancelled in the next cancellation entrypoint > due it is marked as CANCELED_BIT. The __syscall_cancel_arch will check > for the bit and call __do_cancel before issuing the syscall. Sure, but we can exit the SIGCANCEL handler if we detect that cancellation is already in progress. >>>> 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. > > I shared your concern, but my understanding of the whole BZ#12683 > issue is we can't really cancel the thread for the (B) case. Even if we undo the resource allocation before that? That is, check that a cancellation has occurred on the system call return path, and if it has, close the descriptor and start unwinding. >> 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. > > We can hardness the test to check for open failures (due file descriptor > exhaustion or other arcane issue), but the point of the test is fd is > always for the *close* call. If the open syscall is interrupted before the > file descriptor is actually opened, the thread will be cancelled before > close call. Otherwise, the kernel will have to return a valid value > and indicate on the PC from the ucontext_t that the syscall has side-effect. My point is that the test should still be valid without the close call because the open is always canceled according to POSIX. >> 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. > > That's not the interpretation I see, the cancellation description for > such cases is afaiu: > > "The side-effects of acting upon a cancellation request while suspended > during a call of a function are the same as the side-effects that may be > seen in a single-threaded program when a call to a function is interrupted > by a signal and the given function returns [EINTR]" > > And on signal concepts: > > "[EINTR] > Interrupted function call. An asynchronous signal was caught by the process > during the execution of an interruptible function. If the signal handler > performs a normal return, the interrupted function call may return this > condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)." > > And my understanding is, since glibc implemented cancellation using signals > and it requires to use SA_RESTART we can actually return to user if the > syscall has side-effects visible to caller. The open function cannot allocate a new file descriptor and fail with EINTR. So I don't see how this case applies here. Sorry. >>>> (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? > > My understanding is the cancellation handler for close won't interrupt the > syscall, regardless SA_RESTART. What do you mean by “interrupt” here? That close will not return with EINTR? Or that the signal handler does not run? > But indeed it might be the case where the > signal is acted just before the syscall is issue and after the test for > CANCELED_BITMASK is done. In this case the thread will be cancelled and > the descriptor leaked. The musl handler SYS_close differently by not using > the syscall bridge, so cancellation can't be acted upon and so close always > succeed. That seems reasonable. If we check for cancellation after the close system call (successful or not), then I think we have a compliant, leak-free close implementation. Thanks, Florian
On Fri, Aug 23, 2019 at 02:47:39PM +0200, Florian Weimer wrote: > I looked at this some more. It seems that in theory, we could also see > EPIPE in the old implementation or at least observe execution of a > signal handler, and the asynchronous cancel hides that. > > 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. > > 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 (A), I think POSIX allows sufficient wiggle room so that we can > exhibit a partial side effect and still act on the cancellation (without > reporting a partial write first to the application). No. POSIX is very clear that cancellation is only permitted to be acted on when the side effects are the same as EINTR, which can only happen for read/write when no data has been transferred yet. If data has already been transferred, the only option not ruled out by one or more constraints is to return successfully with a short read or write; pending cancellation will then be handled on a subsequent call to a cancellation point. (If you're doing a retry loop until the total length n is transferred, then in the next iteration of the loop.) Of course this case of non-conformance would be less severe than UAF or resource leak errors in most cases, but it's still not good. > For (B), maybe we should undo the resource allocation and then proceed > to act on the cancellation. No, why would you do that? In general it's not possible (think of things like O_TRUNC), and even if it were it's more work. You just don't act on the cancellation if success has already happened. I don't see how/why you're characterizing "accept4" as "can easily be reverted". Closing the accepted socket would *drop a connection* rather than leaving the connection pending for another thread calling accept to receive. > 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. > > (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. Failure to specify EINTR was an error in POSIX that's been fixed for the next issue. Leaving it undefined is atrociously bad and results in UAF type errors. Even if glibc doesn't want to apply the fix for EINTR, cancellation of close absolutely needs to behave *as if* by the newly-specified behavior for close with EINTR. Rich
On 30/08/2019 08:51, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 26/08/2019 07:06, Florian Weimer wrote: >>> * 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.) > >> The setjmp does not really matter here, once the async-cancellation fails >> to act due side-effects SIGCANCEL is removed from thread signal mask. The >> idea of making it sticky is always allow the thread to act on such cases >> regardless whether it calls another cancellation entrypoint. > > I see. But why are we doing this? Can't we suppress the cancel signal > inside pthread_cancel. Then we don't have to worry about the location > of the signal mask in the signal context. I guess we can, it would require a sigprocmask to obtain the current mask, and another one to remove the cancellation signal. I still think it cleaner to just sets on the signal handler, since the kernel will restore the signal handler after the userspace returns. > >> The thread will still be cancelled in the next cancellation entrypoint >> due it is marked as CANCELED_BIT. The __syscall_cancel_arch will check >> for the bit and call __do_cancel before issuing the syscall. > > Sure, but we can exit the SIGCANCEL handler if we detect that > cancellation is already in progress. I think it is possible, but I see that enforcing it using the kernel facilities to avoid extra code in userspace is cleaner. > >>>>> 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. >> >> I shared your concern, but my understanding of the whole BZ#12683 >> issue is we can't really cancel the thread for the (B) case. > > Even if we undo the resource allocation before that? That is, check > that a cancellation has occurred on the system call return path, and if > it has, close the descriptor and start unwinding. What if the application requires extra steps for the case the syscall returns a side-effects? What about some specific syscalls, such as passing file descriptor using recvmsg? It would need the libc to map *all* possible side-effects for each syscall, determine which might leak side-effect and acts uppon then. What if the kernel, at some version, added another feature to share resources that might create side-effects? We will need to constantly map such cases, adding even more arch-specific glue code to handle it. I really think we should not go on this path. > >>> 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. >> >> We can hardness the test to check for open failures (due file descriptor >> exhaustion or other arcane issue), but the point of the test is fd is >> always for the *close* call. If the open syscall is interrupted before the >> file descriptor is actually opened, the thread will be cancelled before >> close call. Otherwise, the kernel will have to return a valid value >> and indicate on the PC from the ucontext_t that the syscall has side-effect. > > My point is that the test should still be valid without the close call > because the open is always canceled according to POSIX. > >>> 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. >> >> That's not the interpretation I see, the cancellation description for >> such cases is afaiu: >> >> "The side-effects of acting upon a cancellation request while suspended >> during a call of a function are the same as the side-effects that may be >> seen in a single-threaded program when a call to a function is interrupted >> by a signal and the given function returns [EINTR]" >> >> And on signal concepts: >> >> "[EINTR] >> Interrupted function call. An asynchronous signal was caught by the process >> during the execution of an interruptible function. If the signal handler >> performs a normal return, the interrupted function call may return this >> condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)." >> >> And my understanding is, since glibc implemented cancellation using signals >> and it requires to use SA_RESTART we can actually return to user if the >> syscall has side-effects visible to caller. > > The open function cannot allocate a new file descriptor and fail with > EINTR. So I don't see how this case applies here. Sorry. But that is *exactly* the point. The open *does not* fail, but cancellation acts *before* it can return to the callee. We can't really make it fail with EINTR and acts on the cancellation. The idea is "the interrupted function call may return this condition". > >>>>> (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? >> >> My understanding is the cancellation handler for close won't interrupt the >> syscall, regardless SA_RESTART. > > What do you mean by “interrupt” here? That close will not return with > EINTR? Or that the signal handler does not run? My understanding on the discussions is Linux close implementation does not return EINTR/-1 in any case (and if it did in the past it was a bug). > >> But indeed it might be the case where the >> signal is acted just before the syscall is issue and after the test for >> CANCELED_BITMASK is done. In this case the thread will be cancelled and >> the descriptor leaked. The musl handler SYS_close differently by not using >> the syscall bridge, so cancellation can't be acted upon and so close always >> succeed. > > That seems reasonable. > > If we check for cancellation after the close system call (successful or > not), then I think we have a compliant, leak-free close implementation. In my understanding musl does this to prevent possible problematic cancel syscall implementation that might return -1/EINTR (since for this case syscall will be restart due SA_RESTART and cancellation won't acted upon).
* Rich Felker: > On Fri, Aug 23, 2019 at 02:47:39PM +0200, Florian Weimer wrote: >> I looked at this some more. It seems that in theory, we could also see >> EPIPE in the old implementation or at least observe execution of a >> signal handler, and the asynchronous cancel hides that. >> >> 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. >> >> 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 (A), I think POSIX allows sufficient wiggle room so that we can >> exhibit a partial side effect and still act on the cancellation (without >> reporting a partial write first to the application). > > No. POSIX is very clear that cancellation is only permitted to be > acted on when the side effects are the same as EINTR, which can only > happen for read/write when no data has been transferred yet. If data > has already been transferred, the only option not ruled out by one or > more constraints is to return successfully with a short read or write; > pending cancellation will then be handled on a subsequent call to a > cancellation point. (If you're doing a retry loop until the total > length n is transferred, then in the next iteration of the loop.) This seems to be a backwards interpretation to me. I think POSIX wants to describe what can happen during cancellation, and not say that any cancellation point can be skipped if some side effect happens that cannot happen before EINTR. > Of course this case of non-conformance would be less severe than UAF > or resource leak errors in most cases, but it's still not good. Agreed. And I think not acting on the cancellation and returning success is also not conforming. The main problem is that POSIX would need to specify a memory model in order to make the current language (“side-effects occur before any cancellation cleanup handlers are called”, “a thread is canceled while executing the function”, “a cancellation request has been made”, etc.). >> For (B), maybe we should undo the resource allocation and then proceed >> to act on the cancellation. > > No, why would you do that? In general it's not possible (think of > things like O_TRUNC), and even if it were it's more work. You just > don't act on the cancellation if success has already happened. The problem is that the implementation reorders cancellation requests fairly arbitrarily. I don't think that's what the authors of POSIX had in mind when they said that the cancellation itself progresses asynchronously. I don't think that can be fixed without kernel support, though. We'll see eventually how this impacts existing applications. > I don't see how/why you're characterizing "accept4" as "can easily be > reverted". Closing the accepted socket would *drop a connection* > rather than leaving the connection pending for another thread calling > accept to receive. And this is true for open as well. For FIFOs, there is an observable side effect, likewise for rewinding tape devices, and even for regular files, the close call affects POSIX advisory locks on the file. Oh well. So it's just not possible to unwind directly after file descriptor allocation. >> 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. >> >> (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. > > Failure to specify EINTR was an error in POSIX that's been fixed for > the next issue. Leaving it undefined is atrociously bad and results in > UAF type errors. Even if glibc doesn't want to apply the fix for > EINTR, cancellation of close absolutely needs to behave *as if* by the > newly-specified behavior for close with EINTR. Can we even make sure we act on the cancellation only if the descriptor has been closed? Thanks, Florian
* Adhemerval Zanella: > I think it is possible, but I see that enforcing it using the kernel > facilities to avoid extra code in userspace is cleaner. Okay. >> Even if we undo the resource allocation before that? That is, check >> that a cancellation has occurred on the system call return path, and if >> it has, close the descriptor and start unwinding. > > What if the application requires extra steps for the case the syscall > returns a side-effects? What about some specific syscalls, such as > passing file descriptor using recvmsg? It would need the libc to map > *all* possible side-effects for each syscall, determine which might > leak side-effect and acts uppon then. What if the kernel, at some version, > added another feature to share resources that might create side-effects? > We will need to constantly map such cases, adding even more arch-specific > glue code to handle it. > > I really think we should not go on this path. I agree now, given what we've got from the kernel. See my response to Rich. >>> That's not the interpretation I see, the cancellation description for >>> such cases is afaiu: >>> >>> "The side-effects of acting upon a cancellation request while suspended >>> during a call of a function are the same as the side-effects that may be >>> seen in a single-threaded program when a call to a function is interrupted >>> by a signal and the given function returns [EINTR]" >>> >>> And on signal concepts: >>> >>> "[EINTR] >>> Interrupted function call. An asynchronous signal was caught by the process >>> during the execution of an interruptible function. If the signal handler >>> performs a normal return, the interrupted function call may return this >>> condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)." >>> >>> And my understanding is, since glibc implemented cancellation using signals >>> and it requires to use SA_RESTART we can actually return to user if the >>> syscall has side-effects visible to caller. >> >> The open function cannot allocate a new file descriptor and fail with >> EINTR. So I don't see how this case applies here. Sorry. > > But that is *exactly* the point. The open *does not* fail, but > cancellation acts *before* it can return to the callee. We can't > really make it fail with EINTR and acts on the cancellation. The idea > is "the interrupted function call may return this condition". My uneasiness stems from the fact that we're reading POSIX backwards, i.e., our interpretation (necessarily, given the kernel interfaces) behaves in a way that is obviously to complying with a direct reading of POSIX (a to-be-canceled thread synchronizing with the canceling thread, when the pthread_cancel call happened before the synchronization in program order in the canceling thread), and we then proceed to find reasons why this not totally broken. So, back to the start of the thread: Can we avoid the EPIPE error in an other way? Can we remove this line? /* This will cause the write in the child to return. */ close (fd[0]); Cancellation should still happen if the write is blocked, right? It's not really clear to me what the intent of the test was. Thanks, Florian
On 02/09/2019 13:37, Florian Weimer wrote: > * Rich Felker: >> No. POSIX is very clear that cancellation is only permitted to be >> acted on when the side effects are the same as EINTR, which can only >> happen for read/write when no data has been transferred yet. If data >> has already been transferred, the only option not ruled out by one or >> more constraints is to return successfully with a short read or write; >> pending cancellation will then be handled on a subsequent call to a >> cancellation point. (If you're doing a retry loop until the total >> length n is transferred, then in the next iteration of the loop.) > > This seems to be a backwards interpretation to me. I think POSIX wants > to describe what can happen during cancellation, and not say that any > cancellation point can be skipped if some side effect happens that > cannot happen before EINTR. > >> Of course this case of non-conformance would be less severe than UAF >> or resource leak errors in most cases, but it's still not good. > > Agreed. And I think not acting on the cancellation and returning > success is also not conforming. how is that non-conformance observable? the user cannot know if the cancellation request arrived in time or not if io error (short write) can happen for other reasons than the cancellation signal.
On Mon, Sep 02, 2019 at 02:37:17PM +0200, Florian Weimer wrote: > * Rich Felker: > > > On Fri, Aug 23, 2019 at 02:47:39PM +0200, Florian Weimer wrote: > >> I looked at this some more. It seems that in theory, we could also see > >> EPIPE in the old implementation or at least observe execution of a > >> signal handler, and the asynchronous cancel hides that. > >> > >> 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. > >> > >> 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 (A), I think POSIX allows sufficient wiggle room so that we can > >> exhibit a partial side effect and still act on the cancellation (without > >> reporting a partial write first to the application). > > > > No. POSIX is very clear that cancellation is only permitted to be > > acted on when the side effects are the same as EINTR, which can only > > happen for read/write when no data has been transferred yet. If data > > has already been transferred, the only option not ruled out by one or > > more constraints is to return successfully with a short read or write; > > pending cancellation will then be handled on a subsequent call to a > > cancellation point. (If you're doing a retry loop until the total > > length n is transferred, then in the next iteration of the loop.) > > This seems to be a backwards interpretation to me. I think POSIX wants > to describe what can happen during cancellation, and not say that any > cancellation point can be skipped if some side effect happens that > cannot happen before EINTR. The exact text is: "The side-effects of acting upon a cancellation request while suspended during a call of a function are the same as the side-effects that may be seen in a single-threaded program when a call to a function is interrupted by a signal and the given function returns [EINTR]." Admittedly it took me a while to understand this back when I first started investigating the problem in 2010, but based on both careful reading of this text, and on reasoning about what conditions are *absolutely necessary* to make the cancellation functionality usable in any meaningful way (without race conditions), yes it means that if you act on cancellation, the [only] side effects the cancelled function can have are side effects that would be allowed if the function had failed with EINTR. In hindsight, the main obstacle to my understanding this was bias based on how badly wrong all existing implementations I'd seen were, and resistance to believing they were all so badly wrong. But they were/are. Moreover, the specification gives the implementation liberty *not* to act on cancellation in certain conditions (very end of XSH 2.9.5 Thread Cancellation, subheading Cancellation points): "It is unspecified whether the cancellation request is acted upon or whether the cancellation request remains pending and the thread resumes normal execution if: - The thread is suspended at a cancellation point and the event for which it is waiting occurs - A specified timeout expired before the cancellation request is acted upon." The way I read this is that, if you start processing a cancellation request, but see that the timeout has already expired or that whatever the function was blocked waiting for has already happened, you can abort cancellation and leave it pending and return the result. In an implementation where libc/libpthread cancellation and the kernel are tightly coupled, this is really a choice -- for example the kernel could check at the last point before success during SYS_read to see that cancellation is pending, and *not actually transfer the data*, but instead initiate cancellation with no side effects on the open file. But on an implementation like Linux where libc/libpthread and the cancellation implementation is completely decoupled from the kernel and implemented via signals (which is a hack!), you don't actually have this freedom. By the time you see the signal, it's possible that the syscall *already succeeded* with (potentially serious) side effects, and in that case you have to use the option POSIX gives you here to defer cancellation until the next cancellation point. (If POSIX did not give you this option, then it may not even be possible to do a conforming implementation based on the signal hack). > > Of course this case of non-conformance would be less severe than UAF > > or resource leak errors in most cases, but it's still not good. > > Agreed. And I think not acting on the cancellation and returning > success is also not conforming. It is conforming, by the above-quoted passage. > >> 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. > >> > >> (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. > > > > Failure to specify EINTR was an error in POSIX that's been fixed for > > the next issue. Leaving it undefined is atrociously bad and results in > > UAF type errors. Even if glibc doesn't want to apply the fix for > > EINTR, cancellation of close absolutely needs to behave *as if* by the > > newly-specified behavior for close with EINTR. > > Can we even make sure we act on the cancellation only if the descriptor > has been closed? If the descriptor has been closed, you *can't* act on cancellation. On Linux, SYS_close *never* returns to userspace without the fd having been closed. It can return EINTR, indicating that the fd has been closed but the underlying open file description (if this was the last reference to it) has not, but this EINTR is not POSIX conforming with the new changes to POSIX, and it was always inconsistent with the semantics implied by EINTR ("no nontrivial side effects have happened yet"). See https://sourceware.org/bugzilla/show_bug.cgi?id=14627 In any case, this is easy to deal with for cancellation, even if fixing the EINTR issue is controversial. If PC is before or at the syscall instruction, nothing has happened yet (fd is still open) and you can act on cancellation. If PC is after the syscall instruction, you can't act on it. This is actually the same as all other syscalls, in some sense, except that for (at least a few) other syscalls you want to also act on cancellation if EINTR is returned, because a few of them EINTR on receipt of any signal, including SA_RESTART ones like SIGCANCEL. Historically there were lots like this, but most of them were conformance bugs and fixed. I think select and/or poll is still like that intentionally, but I may be misremembering. Rich
On 02/09/2019 20:26, Rich Felker wrote: > If the descriptor has been closed, you *can't* act on cancellation. On > Linux, SYS_close *never* returns to userspace without the fd having > been closed. It can return EINTR, indicating that the fd has been > closed but the underlying open file description (if this was the last > reference to it) has not, but this EINTR is not POSIX conforming with > the new changes to POSIX, and it was always inconsistent with the > semantics implied by EINTR ("no nontrivial side effects have happened > yet"). See https://sourceware.org/bugzilla/show_bug.cgi?id=14627 So is it true even for newer version? I had the impression from the lwn article [1] that this is driver dependent and this behaviour will eventually be removed from kernel to not allow close to return EINTR. About BZ#14627, does it still make sense for *Linux* to map EINTR to EINPROGRESS? [1] https://lwn.net/Articles/576478/
On Tue, Sep 03, 2019 at 09:15:58AM -0300, Adhemerval Zanella wrote: > > > On 02/09/2019 20:26, Rich Felker wrote: > > If the descriptor has been closed, you *can't* act on cancellation. On > > Linux, SYS_close *never* returns to userspace without the fd having > > been closed. It can return EINTR, indicating that the fd has been > > closed but the underlying open file description (if this was the last > > reference to it) has not, but this EINTR is not POSIX conforming with > > the new changes to POSIX, and it was always inconsistent with the > > semantics implied by EINTR ("no nontrivial side effects have happened > > yet"). See https://sourceware.org/bugzilla/show_bug.cgi?id=14627 > > So is it true even for newer version? I had the impression from the lwn > article [1] that this is driver dependent and this behaviour will eventually > be removed from kernel to not allow close to return EINTR. > > About BZ#14627, does it still make sense for *Linux* to map EINTR to > EINPROGRESS? > > [1] https://lwn.net/Articles/576478/ The kernel folks are adamantly opposed to changing SYS_close to match the POSIX requirement here, which is the right thing to do but for the wrong reasons -- if it were changed, libc could not safely handle the situation because it couldn't know what meaning the kernel was assigning to EINTR. Hopefully they will just stop returning EINTR entirely. That would not break any of the behavior discussed above. Returning EINPROGRESS may be a problem though because applications may not know to expect it and may treat it as a hard error rather than a reasonable condition (discussed in https://sourceware.org/bugzilla/show_bug.cgi?id=14627). The ideal behavior is probably just returning 0. Rich
On 02/09/2019 09:51, Florian Weimer wrote: > * Adhemerval Zanella: > >> I think it is possible, but I see that enforcing it using the kernel >> facilities to avoid extra code in userspace is cleaner. > > Okay. > >>> Even if we undo the resource allocation before that? That is, check >>> that a cancellation has occurred on the system call return path, and if >>> it has, close the descriptor and start unwinding. >> >> What if the application requires extra steps for the case the syscall >> returns a side-effects? What about some specific syscalls, such as >> passing file descriptor using recvmsg? It would need the libc to map >> *all* possible side-effects for each syscall, determine which might >> leak side-effect and acts uppon then. What if the kernel, at some version, >> added another feature to share resources that might create side-effects? >> We will need to constantly map such cases, adding even more arch-specific >> glue code to handle it. >> >> I really think we should not go on this path. > > I agree now, given what we've got from the kernel. See my response to > Rich. > >>>> That's not the interpretation I see, the cancellation description for >>>> such cases is afaiu: >>>> >>>> "The side-effects of acting upon a cancellation request while suspended >>>> during a call of a function are the same as the side-effects that may be >>>> seen in a single-threaded program when a call to a function is interrupted >>>> by a signal and the given function returns [EINTR]" >>>> >>>> And on signal concepts: >>>> >>>> "[EINTR] >>>> Interrupted function call. An asynchronous signal was caught by the process >>>> during the execution of an interruptible function. If the signal handler >>>> performs a normal return, the interrupted function call may return this >>>> condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)." >>>> >>>> And my understanding is, since glibc implemented cancellation using signals >>>> and it requires to use SA_RESTART we can actually return to user if the >>>> syscall has side-effects visible to caller. >>> >>> The open function cannot allocate a new file descriptor and fail with >>> EINTR. So I don't see how this case applies here. Sorry. >> >> But that is *exactly* the point. The open *does not* fail, but >> cancellation acts *before* it can return to the callee. We can't >> really make it fail with EINTR and acts on the cancellation. The idea >> is "the interrupted function call may return this condition". > > My uneasiness stems from the fact that we're reading POSIX backwards, > i.e., our interpretation (necessarily, given the kernel interfaces) > behaves in a way that is obviously to complying with a direct reading of > POSIX (a to-be-canceled thread synchronizing with the canceling thread, > when the pthread_cancel call happened before the synchronization in > program order in the canceling thread), and we then proceed to find > reasons why this not totally broken. > > So, back to the start of the thread: Can we avoid the EPIPE error in an > other way? > > Can we remove this line? > > /* This will cause the write in the child to return. */ > close (fd[0]); > > Cancellation should still happen if the write is blocked, right? It's > not really clear to me what the intent of the test was. I think Rich has answered it better than me that using signals to implement thread cancellation is an implementation detail and POSIX allows us for this case to not act on some cancellation signals if it leads to visible side-effects that might lead to resource leakage. Said that, it would be possible to just remove the close call to avoid the EPIPE error on write. The cancellation should still happen because the write is called on a loop, so since it might return with a visible side-effect (partial write), it would be cancelled in the next write call due the thread being marked as cancelled. I really don't have a strong opinion which change is the better, my point is the current code is racy and it will start to fail more often once we fix BZ#12683.
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; }