__pthread_setcancelstate called unconditionally, crashes at 0

__pthread_setcancelstate called unconditionally, crashes at 0

[Sergey Bugaev]

Hello,

I'm hitting a crash with the following backtrace:

#0  0x0000000000000000 in ?? ()
#1  0x00000000004660dd in __error_internal (status=1,
errnum=1073741826, message=0x9adcef1c, args=0x9adcef18,
args@entry=0x156aa48, mode_flags=2598170400, mode_flags@entry=0) at
error.c:243
#2  0x00000000004661d5 in __error (status=status@entry=0,
errnum=<optimized out>, message=<optimized out>) at error.c:274
#3  0x0000000000401497 in error (__format=<optimized out>,
__errnum=<optimized out>, __status=<optimized out>) at error.h:42

Note that PC is 0 in the top frame. Here's the relevant listing and
backtrace of the frame #1:

(gdb) l
238 {
239 #if defined _LIBC
240  /* We do not want this call to be cut short by a thread
241     cancellation.  Therefore disable cancellation for now.  */
242  int state = PTHREAD_CANCEL_ENABLE;
243  __pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, &state);
244 #endif
245
246  flush_stdout ();
247 #ifdef _LIBC

(gdb) disas
Dump of assembler code for function __error_internal:
   0x00000000004660b0 <+0>: push   %r14
   0x00000000004660b2 <+2>: mov    %r8d,%r14d
   0x00000000004660b5 <+5>: push   %r13
   0x00000000004660b7 <+7>: mov    %rcx,%r13
   0x00000000004660ba <+10>: push   %r12
   0x00000000004660bc <+12>: mov    %rdx,%r12
   0x00000000004660bf <+15>: push   %rbp
   0x00000000004660c0 <+16>: mov    %esi,%ebp
   0x00000000004660c2 <+18>: push   %rbx
   0x00000000004660c3 <+19>: mov    %edi,%ebx
   0x00000000004660c5 <+21>: xor    %edi,%edi
   0x00000000004660c7 <+23>: sub    $0x10,%rsp
   0x00000000004660cb <+27>: lea    0xc(%rsp),%rsi
   0x00000000004660d0 <+32>: movl   $0x1,0xc(%rsp)
   0x00000000004660d8 <+40>: call   0x0
=> 0x00000000004660dd <+45>: mov    $0x569b10,%rax
   0x00000000004660e4 <+52>: mov    (%rax),%rdi
   0x00000000004660e7 <+55>: call   0x433900 <_IO_fflush>

"call 0x0", ouch!

Clearly __pthread_setcancelstate has been pragma weak'd, and used here
without a check. This is a statically linked x86_64-gnu (so, Hurd and
HTL) executable. Commit 93d78ec1cba68184931b75bef29afd3aed30f43a
"nptl: Move pthread_setcancelstate into libc" seems to be the culprit:
that commit only moved the NPTL symbol into libc, yet changed the
original __libc_ptf_call (__pthread_setcancelstate) calls to direct
__pthread_setcancelstate calls, in this and many other places.

This likely hasn't been noticed in the past because the only
statically linked executables typically used on Hurd systems are the
few bootstrap servers, and they're (presumably) all multithreaded.

What would be the best way to get this fixed? (other than eventually
moving htl into libc) Are the other pthread symbols also used
unconditionally? Is there, or should there be, a test for this?

Sergey

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Florian Weimer]

* Sergey Bugaev:

> Clearly __pthread_setcancelstate has been pragma weak'd, and used here
> without a check. This is a statically linked x86_64-gnu (so, Hurd and
> HTL) executable. Commit 93d78ec1cba68184931b75bef29afd3aed30f43a
> "nptl: Move pthread_setcancelstate into libc" seems to be the culprit:
> that commit only moved the NPTL symbol into libc, yet changed the
> original __libc_ptf_call (__pthread_setcancelstate) calls to direct
> __pthread_setcancelstate calls, in this and many other places.

Apparently, Hurd does not support async cancellation?  Then
__pthread_setcancelstate never has to unwind, so you just turn it into a
non-weak symbol.

If you need async cancellation support, the core cancellation routine
could be made weak, so that it is linked into the executable only if
pthread_cancel is ever called.

Thanks,
Florian

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Samuel Thibault]

Florian Weimer, le jeu. 11 mai 2023 19:44:42 +0200, a ecrit:
> * Sergey Bugaev:
> > Clearly __pthread_setcancelstate has been pragma weak'd, and used here
> > without a check. This is a statically linked x86_64-gnu (so, Hurd and
> > HTL) executable. Commit 93d78ec1cba68184931b75bef29afd3aed30f43a
> > "nptl: Move pthread_setcancelstate into libc" seems to be the culprit:
> > that commit only moved the NPTL symbol into libc, yet changed the
> > original __libc_ptf_call (__pthread_setcancelstate) calls to direct
> > __pthread_setcancelstate calls, in this and many other places.
> 
> Apparently, Hurd does not support async cancellation?

? It does, see htl/pt-cancel.c's check for PTHREAD_CANCEL_ASYNCHRONOUS.

Samuel

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Florian Weimer]

* Samuel Thibault:

> Florian Weimer, le jeu. 11 mai 2023 19:44:42 +0200, a ecrit:
>> * Sergey Bugaev:
>> > Clearly __pthread_setcancelstate has been pragma weak'd, and used here
>> > without a check. This is a statically linked x86_64-gnu (so, Hurd and
>> > HTL) executable. Commit 93d78ec1cba68184931b75bef29afd3aed30f43a
>> > "nptl: Move pthread_setcancelstate into libc" seems to be the culprit:
>> > that commit only moved the NPTL symbol into libc, yet changed the
>> > original __libc_ptf_call (__pthread_setcancelstate) calls to direct
>> > __pthread_setcancelstate calls, in this and many other places.
>> 
>> Apparently, Hurd does not support async cancellation?
>
> ? It does, see htl/pt-cancel.c's check for
> PTHREAD_CANCEL_ASYNCHRONOUS.

But __pthread_setcancelstate does not trigger async cancellation if
there's a pending async request and cancellation is enabled.

Thanks,
Florian

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Sergey Bugaev]

On Thu, May 11, 2023 at 8:44 PM Florian Weimer <fweimer@redhat.com> wrote:
> Apparently, Hurd does not support async cancellation? Then
> __pthread_setcancelstate never has to unwind, so you just turn it into a
> non-weak symbol.

It does in theory, htl/pt-cancel.c has a PTHREAD_CANCEL_ASYNCHRONOUS
branch that calls into sysdeps/mach/hurd/htl/pt-docancel.c, which
manipulates the victim thread's state to make it call __pthread_exit
(0). (This is also implemented in a rather sad way that most likely
wouldn't work on x86_64-gnu; I'll get to fixing that some time...)

But it doesn't seem to either do any unwinding, nor is
__pthread_setcancelstate async-cancel-safe (it uses a mutex).

> If you need async cancellation support, the core cancellation routine
> could be made weak, so that it is linked into the executable only if
> pthread_cancel is ever called.

Could you please expand on how this all (unwinding, async
cancellation) is relevant? Clearly calling error () in a
PTHREAD_CANCEL_ASYNCHRONOUS context is undefined behavior since error
() is not async-cancel-safe.

Sergey

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Florian Weimer]

* Sergey Bugaev:

>> If you need async cancellation support, the core cancellation routine
>> could be made weak, so that it is linked into the executable only if
>> pthread_cancel is ever called.
>
> Could you please expand on how this all (unwinding, async
> cancellation) is relevant? Clearly calling error () in a
> PTHREAD_CANCEL_ASYNCHRONOUS context is undefined behavior since error
> () is not async-cancel-safe.

I'd expect __pthread_setcancelstate to act on asynchronous cancellation
if it is enabled.  Once you implement that and have a strong symbol
reference to __pthread_setcancelstate, you'd pull in the cancellation
unwinder without additional measures, and this would into every program.
You probably don't want that.  So the reference to the core cancellation
had better be weak (or use the __libc_ptf_call indirection).

Does this answer your question?  Sorry, these static linking size
optimizations are tricky.

Thanks,
Florian

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Sergey Bugaev]

On Thu, May 11, 2023 at 9:12 PM Florian Weimer <fweimer@redhat.com> wrote:
> I'd expect __pthread_setcancelstate to act on asynchronous cancellation
> if it is enabled.  Once you implement that and have a strong symbol
> reference to __pthread_setcancelstate, you'd pull in the cancellation
> unwinder without additional measures, and this would into every program.
> You probably don't want that.  So the reference to the core cancellation
> had better be weak (or use the __libc_ptf_call indirection).
>
> Does this answer your question?  Sorry, these static linking size
> optimizations are tricky.

I think it does, thank you!

As for unwinding: it appears that that htl does call the cleanup
handlers, but without any actual unwinding. It just does this:

  for (handlers = __pthread_get_cleanup_stack ();
       *handlers != NULL;
       *handlers = (*handlers)->__next)
    (*handlers)->__handler ((*handlers)->__arg);

where __pthread_get_cleanup_stack just returns
&__pthread_cleanup_stack. So if we add this immediate cancellation &
cleanup logic to __pthread_setcancelstate, it shouldn't pull in much.

Do user programs in practice rely on the fact that pthread
cancellation does unwinding and so calls object destructors, runs
@finally clauses, etc?

Sergey

Re: __pthread_setcancelstate called unconditionally, crashes at 0

[Florian Weimer]

* Sergey Bugaev:

> Do user programs in practice rely on the fact that pthread
> cancellation does unwinding and so calls object destructors, runs
> @finally clauses, etc?

I think so, yes.  I don't recall introducing a regression in that part
of cancellation, so we don't have good usage data unfortunately.

Thanks,
Florian