[musl] Stack error through asynchronous cancellation

[musl] Stack error through asynchronous cancellation

[Markus Wichmann]

Hi all,

I noticed that when a thread is cancelled while asynchronous
cancellation is in effect, then execution is redirected to __cp_cancel,
but __cp_cancel is a label only meant to undo the stack manipulation of
__syscall_cp, then jump to __cancel. This means, during asynchronous
cancellation, invalid values will be read from stack, and the stack
pointer may be set to a misaligned value before continuing to
__cp_cancel. For the most part, it will not matter, since __cancel will
not return if asynchronous cancellation is in effect, but there is still
something iffy about this whole behavior.

I wonder if we maybe need a new label __cp_cancel_async, that
transitions to __cancel from unknown legal processor state. On x86_64,
for example, it is possible (though unlikely) that the direction flag is
set, thus leading to undefined behavior when __cancel is invoked with
the flag set, even though the ABI says it is not supposed to be set on
entry to any function.

On architectures that do actually adjust the stack in __syscall_cp, the
changes to the stack pointer could also mean overwriting a thread
cancellation handler. And while pthread_cleanup_push() and ..._pop() are
not async-cancel-safe, can they not be invoked under deferred
cancellation and then have asynchronous cancellation be used between
them?

Ciao,
Markus

Re: [musl] Stack error through asynchronous cancellation

[Rich Felker]

On Mon, Nov 21, 2022 at 09:54:13PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> I noticed that when a thread is cancelled while asynchronous
> cancellation is in effect, then execution is redirected to __cp_cancel,
> but __cp_cancel is a label only meant to undo the stack manipulation of
> __syscall_cp, then jump to __cancel. This means, during asynchronous
> cancellation, invalid values will be read from stack, and the stack
> pointer may be set to a misaligned value before continuing to
> __cp_cancel. For the most part, it will not matter, since __cancel will
> not return if asynchronous cancellation is in effect, but there is still
> something iffy about this whole behavior.
> 
> I wonder if we maybe need a new label __cp_cancel_async, that
> transitions to __cancel from unknown legal processor state. On x86_64,
> for example, it is possible (though unlikely) that the direction flag is
> set, thus leading to undefined behavior when __cancel is invoked with
> the flag set, even though the ABI says it is not supposed to be set on
> entry to any function.
> 
> On architectures that do actually adjust the stack in __syscall_cp, the
> changes to the stack pointer could also mean overwriting a thread
> cancellation handler. And while pthread_cleanup_push() and ..._pop() are
> not async-cancel-safe, can they not be invoked under deferred
> cancellation and then have asynchronous cancellation be used between
> them?

Thanks! This was also reported to me off-list recently, and I have a
fix; I just hadn't pushed it yet.

The behavior of changing the return address is only valid and only
makes sense for synchronous cancellation at a cancellation point. For
async, we just need to go back to the behavior prior to commit
102f6a01e249ce4495f1119ae6d963a2a4a53ce5, where __cancel is called
from the cancellation signal handler. There are other ways but they
would require a lot of arch-specific knowledge (like DF you mentioned)
to construct a valid frame to return into, and that's just not needed.

Rich