Re: [PATCH v2 02/21] nptl: Fix testcases for new pthread cancellation mechanism

[Zack Weinberg <zackw at panix dot com>]

On 26 Feb 2018, Adhemerval Zanella wrote:
> With upcoming fix for BZ#12683, pthread cancellation does not act for:
>
>   1. If syscall is blocked but with some side effects already having
>      taken place (e.g. a partial read or write).
>   2. After the syscall has returned.
>
> The main change is due the fact programs need to act in syscalls with
> side-effects (for instance, to avoid leak of allocated resources or
> handle partial read/write).
>
> This patch changes the NPTL testcase that assumes the old behavior and
> also remove the tst-cancel-wrappers.sh test (which checks for symbols
> that will not exist anymore).

It's OK and expected that we have to adjust test cases that were
expecting the old behavior ...

> For tst-cancel{2,3} case it remove the pipe close because it might
> cause the write syscall to return with side effects if the close is
> executed before the pthread cancel.

... however, this change appears to be wrong.  If cancellation is
broken, these tests will now deadlock rather than failing cleanly.

If I understand correctly, the problem you're trying to avoid is that
the 'tf' thread could conceivably receive the closed-pipe event before
the cancellation signal, even though the 'do_test' thread triggers the
cancellation signal first.  I don't know of any way to fix this 100%,
but I think it would be good enough to use pthread_timedjoin_np to
sleep for a hundred milliseconds or so in the 'do_test' thread, and
then close the pipe if the 'tf' thread is still alive.

(tst-cancel4.c appears to be trying to ensure that cancellations are
pending with a pthread_barrier_t, but as far as I know there's no
guarantee that if a thread does

   pthread_cancel(th);
   pthread_barrier_wait(ba);

where 'th' also waits on 'ba', the SIGCANCEL will actually be
delivered before the barrier unblocks, either.  Feh.)

> It also changes how to call the read syscall on tst-backtrace{5,6}
> to use syscall instead of read cancelable syscall to avoid need to
> handle the cancelable bridge function calls.  It requires a change
> on powerpc syscall implementation to create a stackframe, since
> powerpc backtrace rely on such information.

It doesn't look technically difficult to me to handle an additional
stack frame or two in the trace.  They're always going to be there,
aren't they?  In the new world order, the stack trace will always be

 0  handle_signal
 1  <signal trampoline>
 2  __syscall_cancel_arch
 3  __syscall_cancel
 4  read
 5  fn
 6  fn
 7  fn
 8  do_test

won't it?  I think teaching the backtrace logic about this would be
better than needing to use a raw syscall() and then mess with the
PowerPC implementation of syscall().  I might feel differently about
this change if __read_nocancel were a public API, but it isn't...

> Checked on i686-linux-gnu, x86_64-linux-gnu, x86_64-linux-gnux32,
> aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64le-linux-gnu,
> powerpc-linux-gnu, sparcv9-linux-gnu, and sparc64-linux-gnu.

When you say checked, do you mean you actually ran the test cases, or
did you just compile them (perhaps with a cross-compiler)?

>       * nptl/Makefile [$(run-built-tests) = yes] (tests-special): Remove
>       tst-cancel-wrappers.sh.
>       * nptl/tst-cancel-wrappers.sh: Remove file.

This part is OK.

>       * nptl/tst-cancel4.c (tf_write): Handle cancelled syscall with
>       side-effects.
>       (tf_send): Likewise.

This part is also OK.  I think the test can still fail spuriously due
to races, but this one can't deadlock, at least, so we can live with it.

>       * sysdeps/unix/sysv/linux/powerpc/syscall.S (syscall): Create stack
>       frame.

This ChangeLog entry belongs to patch 1.

zw