Bug 187297 – [linux] ASSERT: Using an alternative signal stack is not supported. Consider disabling the concurrent GC.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Some linux clients are encountering this assertion.  Is there a way to either add alternative signal stack support?

<rdar://problem/41765872>

[Mark Lam]

See assertion in Thread::signalHandlerSuspendResume() in ThreadingPthreads.cpp.

[Yusuke Suzuki]

Why alternative stack is not supported is,

1. when a signal handler with an alternative stack flag is invoked, it's stack pointer is an alternative stack.
2. while executig a signal handler, SIGUSR1 is emitted to suspend the thread
3. Nested signal handler is invoked, where machine context's stack pointer is an alternative stack.
4. stack scan is broken.

I think this is not Linux specific. Even in Darwin, an alternative stack is not supported.

1. when a signal handler with an alternative stack flag is invoked, it's stack pointer is an alternative stack.
2. while executig a signal handler, thread_suspend is executed
3. saved thread context's stack pointer is pointing an alternative stack
4. stack scan is broken.

In Linux, the way to avoid this issue is,

1. Do not use an alternative stack

Due to the above reason, not using an alternative stack just fixes the problem.

2. Add SIGUSR1 to a signal mask when an alternative stack flag is specified.

The root cause of the problem is that SIGUSR1 signal handler is invoked while executing a signal handler with an alternative stack. So setting SIGUSR1 to sa_mask can fix the issue.

I think the current assertion is valid in both cases.
In Darwin, we do not have the way to fix the issue except for not using an alternative stack.

Mark, do you have any idea?

[Mark Lam]

(In reply to Yusuke Suzuki from comment #2)
> ...
> I think this is not Linux specific. Even in Darwin, an alternative stack is
> not supported.
> 
> 1. when a signal handler with an alternative stack flag is invoked, it's
> stack pointer is an alternative stack.
> 2. while executig a signal handler, thread_suspend is executed
> 3. saved thread context's stack pointer is pointing an alternative stack
> 4. stack scan is broken.

According to Thread::getRegisters(), OS(DARWIN) does not depend on the platform registers stashed away by Thread::signalHandlerSuspendResume().  Instead, it uses a thread_get_state() to get the registers of the target thread.  I quite sure that thread_get_state() does not require the target thread to be the current thread.  In fact, OS(DARWIN) suspends the target thread and reads its registers using thread_get_state().  Therefore, thread_get_state() has to be operating on a different thread there.

Hence, this issue only applies to Linux.

[Yusuke Suzuki]

(In reply to Mark Lam from comment #3)
> According to Thread::getRegisters(), OS(DARWIN) does not depend on the
> platform registers stashed away by Thread::signalHandlerSuspendResume(). 
> Instead, it uses a thread_get_state() to get the registers of the target
> thread.  I quite sure that thread_get_state() does not require the target
> thread to be the current thread.  In fact, OS(DARWIN) suspends the target
> thread and reads its registers using thread_get_state().  Therefore,
> thread_get_state() has to be operating on a different thread there.
> 
> Hence, this issue only applies to Linux.

What happens if

1. The target thread starts executing user-defined signal handlers with an alternative stack
2. While executing this handler, we suspend this thread by using thread_suspend.
3. Read the state by using thread_get_state()

When reading the stack pointer at (3), is it pointing to an alternative stack?

[Mark Lam]

(In reply to Yusuke Suzuki from comment #4)
> (In reply to Mark Lam from comment #3)
> > According to Thread::getRegisters(), OS(DARWIN) does not depend on the
> > platform registers stashed away by Thread::signalHandlerSuspendResume(). 
> > Instead, it uses a thread_get_state() to get the registers of the target
> > thread.  I quite sure that thread_get_state() does not require the target
> > thread to be the current thread.  In fact, OS(DARWIN) suspends the target
> > thread and reads its registers using thread_get_state().  Therefore,
> > thread_get_state() has to be operating on a different thread there.
> > 
> > Hence, this issue only applies to Linux.
> 
> What happens if
> 
> 1. The target thread starts executing user-defined signal handlers with an
> alternative stack
> 2. While executing this handler, we suspend this thread by using
> thread_suspend.
> 3. Read the state by using thread_get_state()
> 
> When reading the stack pointer at (3), is it pointing to an alternative
> stack?

Good point.  I don't think OS(DARWIN) uses an alternate signal stack, but I should double check.

[Yusuke Suzuki]

Created attachment 344277 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 344277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review

> Source/WTF/wtf/ThreadingPthreads.cpp:371
> +            Thread::yield();

Should we wait a bit here? idk.

[EWS Watchlist]

Comment on attachment 344277 [details]
Patch

Attachment 344277 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8436661

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 344287 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Mark Lam]

Comment on attachment 344277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review

r=me.  Nice work and very clean.

> Source/WTF/wtf/ThreadingPthreads.cpp:155
> +        // 1. An user-defined signal handler is invoked with an alternative signal stack.

/An user-defined/A user-defined/.

> Source/WTF/wtf/ThreadingPthreads.cpp:158
> +        // 4. A stack pointer saved in a machine context is pointing an alternative signal stack.

I suggest "The stack pointer saved in the machine context will be pointing to the alternative signal stack."

>> Source/WTF/wtf/ThreadingPthreads.cpp:371
>> +            Thread::yield();
> 
> Should we wait a bit here? idk.

I think this is fine because I see that we also use Thread::yield() elsewhere for purposes like this.  Given that the probability of a suspension request coinciding with a user-defined signal handler is rare, I think we don't need to worry about any perf issues here.  We already do a wait() in this loop which is way slower.

[Yusuke Suzuki]

Comment on attachment 344277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344277&action=review

Thanks!

>> Source/WTF/wtf/ThreadingPthreads.cpp:155
>> +        // 1. An user-defined signal handler is invoked with an alternative signal stack.
> 
> /An user-defined/A user-defined/.

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:158
>> +        // 4. A stack pointer saved in a machine context is pointing an alternative signal stack.
> 
> I suggest "The stack pointer saved in the machine context will be pointing to the alternative signal stack."

Fixed.

>>> Source/WTF/wtf/ThreadingPthreads.cpp:371
>>> +            Thread::yield();
>> 
>> Should we wait a bit here? idk.
> 
> I think this is fine because I see that we also use Thread::yield() elsewhere for purposes like this.  Given that the probability of a suspension request coinciding with a user-defined signal handler is rare, I think we don't need to worry about any perf issues here.  We already do a wait() in this loop which is way slower.

OK, sounds reasonable.

[Yusuke Suzuki]

Committed r233613: <https://trac.webkit.org/changeset/233613>