Bug 14744

> I am unsure whether the impact is limited to user error (issuing the kill 
> command manually) or could arise in other ways. In any case, for robustness, I 
> think the signal handler should be a no-op unless pthread_cancel was really 
> called in the application.

That would make problem worse as with that kill would sometimes kill random thread if a cancellation was called in meantime which is harder to detect as offending code would be mostly nop.

If this needs handled then by aborting affected process.

Perhaps I was not clear. I (hopefully obviously) did not mean that there should be some global state "this application has called pthread_cancel" which causes any thread receiving signal 32 or 33 to get cancelled if it's set. The intent was that pthread_cancel should set the cancellation state on its target (in the TCB or thread-local storage), and a thread should never act upon cancellation unless this state is set.

(In reply to Rich Felker from comment #2)
> Perhaps I was not clear. I (hopefully obviously) did not mean that there
> should be some global state "this application has called pthread_cancel"
> which causes any thread receiving signal 32 or 33 to get cancelled if it's
> set. The intent was that pthread_cancel should set the cancellation state on
> its target (in the TCB or thread-local storage), and a thread should never
> act upon cancellation unless this state is set.

The only sanity checking done today is:

  /* Safety check.  It would be possible to call this function for
     other signals and send a signal from another process.  This is not
     correct and might even be a security problem.  Try to catch as
     many incorrect invocations as possible.  */
  if (sig != SIGCANCEL
      || si->si_pid != pid
      || si->si_code != SI_TKILL)
    return;

However the implementation comment seems to indicate desiring support for cancellation from another process?

      /* We are canceled now.  When canceled by another thread this flag
         is already set but if the signal is directly send (internally or
         from another process) is has to be done here.  */
      int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;

I can't imagine under what use case you would support cancelling a thread in a process from another process?

Comments?

(In reply to Carlos O'Donell from comment #3)
> However the implementation comment seems to indicate desiring support for
> cancellation from another process?
> 
>       /* We are canceled now.  When canceled by another thread this flag
>          is already set but if the signal is directly send (internally or
>          from another process) is has to be done here.  */
>       int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> 
> I can't imagine under what use case you would support cancelling a thread in
> a process from another process?

The worrisome bit is the "internally" part which seems to indicate there is some path inside glibc which can't set the target thread's cancellation status to "cancelling" which would make it impossible to determine if you should or should not act on the SIGCANCEL signal (assuming AC-enabled).

Allowing cancellation from other applications is certainly not desirable, any moreso than it would be desirable for one process to call free() on a pointer malloc'd by another process. And anyway, pthread_t is only meaningful within the context of the process it belongs to.

Technically you could use the tgkill syscall directly to intentionally send a cancellation signal to a particular target thread in another process (this would be less insane than sending it to the process and having the kernel randomly deliver it to one thread). Perhaps the original intent of the code was to allow something like this, where users might attempt to cancel a stuck thread as a way to get a process going again. This seems like a really bad idea though (most apps don't use or support cancellation, and cancelling a thread that's not written to be cancelled could cause dangerous misbehavior). If someone really wants to go to that kind of extent to try to "fix" a hung application, gdb is the right tool, and in fact you can use gdb to call pthread_cancel on a particular thread or make the thread itself call pthread_exit.

So, I really don't think there's any sense in trying to allow cancelling threads from outside the process.

(In reply to Rich Felker from comment #5)
> Allowing cancellation from other applications is certainly not desirable,
> any moreso than it would be desirable for one process to call free() on a
> pointer malloc'd by another process. And anyway, pthread_t is only
> meaningful within the context of the process it belongs to.
> 
> Technically you could use the tgkill syscall directly to intentionally send
> a cancellation signal to a particular target thread in another process (this
> would be less insane than sending it to the process and having the kernel
> randomly deliver it to one thread). Perhaps the original intent of the code
> was to allow something like this, where users might attempt to cancel a
> stuck thread as a way to get a process going again. This seems like a really
> bad idea though (most apps don't use or support cancellation, and cancelling
> a thread that's not written to be cancelled could cause dangerous
> misbehavior). If someone really wants to go to that kind of extent to try to
> "fix" a hung application, gdb is the right tool, and in fact you can use gdb
> to call pthread_cancel on a particular thread or make the thread itself call
> pthread_exit.
> 
> So, I really don't think there's any sense in trying to allow cancelling
> threads from outside the process.

I agree.

I will mark this bug as fixed now that on 2.34 (41c72956179a8ed730d1ac8198015934398fe72b) the cancellation handler is only installed when pthread_cancel() is called (as the suggestion from comment #1) and in the signal handler glibc ignores request from other processes.