Re: [musl] Potential deadlock in pthread_kill()

[Rich Felker <dalias@libc.org>]

On Mon, Jun 29, 2020 at 11:19:39PM -0700, Hydro Flask wrote:
> >>        int pthread_kill(pthread_t t, int sig)
> >>        {
> >>                int r;
> >>                LOCK(t->killlock);
> >>                r = t->tid ? -__syscall(SYS_tkill, t->tid, sig)
> >>                        : (sig+0U >= _NSIG ? EINVAL : 0);
> >>                UNLOCK(t->killlock);
> >>                return r;
> >>        }
> >>
> >>Thank you for your attention.
> >
> >Thanks. It looks like this case was overlooked in the pthread_cancel
> >fix that was commit 060ed9367337cbbd59a9e5e638a1c2f460192f25. The
> >possibility of blocking signals was even mentioned there but deemed
> >unnecessary.
> >
> >A simpler/lighter fix might be, before the lock,
> >
> >	if (t==__pthread_self())
> >		return -__syscall(SYS_tkill, t->tid, sig);
> >
> >since no lock is needed if targeting self; t->tid is necessarily valid
> >in that case.
> 
> Just to be clear, this doesn't only occur when calling
> pthread_kill() and using pthread_self() as the target, it can be any
> target thread, as long as it's the same target thread is used in the
> signal handler and in the synchronous context.

How so? If the target is different, the rest of the pthread_kill,
including the unlock, will proceed concurrently with the signal
handler. However you may be able to construct mutual-signaling
deadlock cases.

> Looking at the commit message you references, I think the only fix
> for all cases is to block signals before taking the killlock. If

This might be the case.

> there is a way to avoid the killlock altogether that would also fix
> it. Thanks again for confirming the issue.

It can't be removed without replacing it with something else to
synchronize against possible thread exit.

Rich