Bug 213160 – Signal handlers should have a two phase installation.

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Signal handlers should have a two phase installation.

[Keith Miller]

Created attachment 401817 [details]
Patch

[Keith Miller]

Created attachment 401818 [details]
Patch

[Mark Lam]

Comment on attachment 401818 [details]
Patch

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

Some comments for now.  I'll do a proper review later.

> Source/WTF/ChangeLog:10
> +        With Mach exception handlers installed the OS will send a mach message

comma after "installed" please.

> Source/WTF/ChangeLog:12
> +        When this happens the combination of LLDB and the process JSC is in effectively

comma after "happens" please.

> Source/WTF/ChangeLog:15
> +        Under our new approach we go back to only telling the OS we care about

comma after "approach" please.

> Source/JavaScriptCore/jsc.cpp:3243
> +        Wasm::enableFastMemory();

Please fix the indentation.

[Keith Miller]

Created attachment 401821 [details]
Patch

[Keith Miller]

Created attachment 401822 [details]
Patch

[Mark Lam]

Comment on attachment 401822 [details]
Patch

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

> Source/WTF/ChangeLog:17
> +        Under our new approach, we go back to only telling the OS we care about
> +        these exceptions late but lock down the function pointers early. This way
> +        processes that benefit from our exception handler code are easier to debug.

Given my comment below about what should be done in activateSignalHandlersFor(), I would change this to say:

Under our new approach, we go back to registering threads to listen for exceptions late, but lock down all other details about mach exceptions early (including the function pointers, the exception mask, and the exception port). ...

> Source/WTF/wtf/threads/Signals.cpp:243
> +    kern_return_t result = thread_set_exception_ports(thread.machThread(), activeExceptions, handlers.exceptionPort, EXCEPTION_STATE | MACH_EXCEPTION_CODES, MACHINE_THREAD_STATE);

Let's undo this and keep activeExceptions in the Config (see details below).

> Source/WTF/wtf/threads/Signals.cpp:279
> +static std::once_flag initializeOnceFlags[static_cast<size_t>(Signal::NumberOfSignals)];

Why move this outside of addSignalHandler?  No one else needs it.

> Source/WTF/wtf/threads/Signals.cpp:-295
> -        Config::AssertNotFrozenScope assertScope;

Please keep this.  It defends against bugs where we end up jumping into this lambda function.  We want to make sure that this lambda function is never called once we freeze the Config.

> Source/WTF/wtf/threads/Signals.cpp:317
> +    ASSERT(signal < Signal::Unknown);
> +    ASSERT(!handlers.useMach || signal != Signal::Usr);

Make these ASSERT_UNUSED so that !HAVE(MACH_EXCEPTIONS) builds won't complain about an unused arguments and variables.

> Source/WTF/wtf/threads/Signals.cpp:319
> +
> +

Remove excess empty lines.

> Source/WTF/wtf/threads/Signals.cpp:324
> +        activeExceptions |= toMachMask(signal);

Why can't you set activeExceptions in addSignalHandler()?  If a bug overwrites this and removes a Signal type, we may inadvertently disable signal handling that we're depending on.

Let's do this:
1. keep the activeExceptions mask in the Config's handlers record.
2. In addSignalHandler(), set `handlers.activeExceptions |= toMachMask(signal);`
3. In activateSignalHandlersFor(), do this instead: `RELEASE_ASSERT(handlers.activeExceptions & toMachMask(signal));`.  This is ensure that addSignalHandler() was called for this Signal type as expected.

After reading thru this patch, I'm convinced that the only real work that activateSignalHandlersFor() needs to do is simply to register the threads by calling setExceptionPorts() on them.

[Mark Lam]

Comment on attachment 401822 [details]
Patch

r- for now since EWS bots are red anyway.  Please revise the patch.

[Keith Miller]

Comment on attachment 401822 [details]
Patch

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

>> Source/WTF/wtf/threads/Signals.cpp:279
>> +static std::once_flag initializeOnceFlags[static_cast<size_t>(Signal::NumberOfSignals)];
> 
> Why move this outside of addSignalHandler?  No one else needs it.

sure.

>> Source/WTF/wtf/threads/Signals.cpp:-295
>> -        Config::AssertNotFrozenScope assertScope;
> 
> Please keep this.  It defends against bugs where we end up jumping into this lambda function.  We want to make sure that this lambda function is never called once we freeze the Config.

Ah yeah, that got incidentally deleted while moving code around. I'll put it back.

>> Source/WTF/wtf/threads/Signals.cpp:317
>> +    ASSERT(!handlers.useMach || signal != Signal::Usr);
> 
> Make these ASSERT_UNUSED so that !HAVE(MACH_EXCEPTIONS) builds won't complain about an unused arguments and variables.

sure.

>> Source/WTF/wtf/threads/Signals.cpp:324
>> +        activeExceptions |= toMachMask(signal);
> 
> Why can't you set activeExceptions in addSignalHandler()?  If a bug overwrites this and removes a Signal type, we may inadvertently disable signal handling that we're depending on.
> 
> Let's do this:
> 1. keep the activeExceptions mask in the Config's handlers record.
> 2. In addSignalHandler(), set `handlers.activeExceptions |= toMachMask(signal);`
> 3. In activateSignalHandlersFor(), do this instead: `RELEASE_ASSERT(handlers.activeExceptions & toMachMask(signal));`.  This is ensure that addSignalHandler() was called for this Signal type as expected.
> 
> After reading thru this patch, I'm convinced that the only real work that activateSignalHandlersFor() needs to do is simply to register the threads by calling setExceptionPorts() on them.

I think that's a riskier change because then all signals get turned on at once. Until we have a real fix for LLDB I want to make sure we only have the handlers installed when we actually need them.

I don't think the badness from accidentally clearing the bits is that bad since, IIRC, the API will just leave the old handler in place. But even calling setExceptionPorts disables the other handler it's just gonna be a crash because no there's no handler.

[Keith Miller]

Created attachment 401844 [details]
Patch

[Mark Lam]

(In reply to Keith Miller from comment #8)
> > After reading thru this patch, I'm convinced that the only real work that activateSignalHandlersFor() needs to do is simply to register the threads by calling setExceptionPorts() on them.
> 
> I think that's a riskier change because then all signals get turned on at
> once. Until we have a real fix for LLDB I want to make sure we only have the
> handlers installed when we actually need them.
> 
> I don't think the badness from accidentally clearing the bits is that bad
> since, IIRC, the API will just leave the old handler in place. But even
> calling setExceptionPorts disables the other handler it's just gonna be a
> crash because no there's no handler.

After thinking about this more, I agree that subtracting masks is not an issue because it means we will just crash.  However, I think adding masks can still be an issue.  I'll propose a change in your patch.

[Mark Lam]

Comment on attachment 401844 [details]
Patch

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

r=me with fixes for limiting what signals can be activated.

> Source/WTF/ChangeLog:13
> +        LLDB does not like it when we single step while there are mach exception
> +        handlers installed because LLDB suspends all the non-active threads.
> +        With Mach exception handlers installed, the OS will send a mach message
> +        to our exception handler port, which is different than the active thread.
> +        When this happens, the combination of LLDB and the process JSC is in effectively
> +        deadlock.

It's not clear to me how this patch fixes the LLDB deadlock?  At some point, we will call setExceptionPort() for the signals we wish to handle.  How does having delayed the call to setExceptionPort() help after that it does get called?  Wouldn't LLDB also still deadlock then?  Or are you saying that you're only reducing the probability of deadlock here and not actually fixing it completely?  Can you comment and clarify this point in the ChangeLog.

Anyway, I think this patch is an improvement.

> Source/WTF/wtf/threads/Signals.cpp:243
> +    kern_return_t result = thread_set_exception_ports(thread.machThread(), activeExceptions, handlers.exceptionPort, EXCEPTION_STATE | MACH_EXCEPTION_CODES, MACHINE_THREAD_STATE);

Replace "activeExceptions" with "activeExceptions & handlers.addedExceptions" here.

> Source/WTF/wtf/threads/Signals.cpp:-295
> -        Config::AssertNotFrozenScope assertScope;

Please restore this Config::AssertNotFrozenScope.

> Source/WTF/wtf/threads/Signals.cpp:323
> -        handlers.activeExceptions |= toMachMask(signal);
> +        activeExceptions |= toMachMask(signal);

To ensure that we don't allow activating any handlers for signals that we didn't explicitly call addSignalHandler() for, let's do the following:
1. In SignalHandlers::add(), please add the following:
        addedExceptions |= toMachMask(signal);
2. Here in activateSignalHandlersFor(), change this line to:
        activeExceptions |= (toMachMask(signal) & handlers.addedExceptions);

> Source/WTF/wtf/threads/Signals.h:112
>      exception_mask_t activeExceptions;

Rename this "activeExceptions" field to to "addedExceptions".

[Keith Miller]

Comment on attachment 401844 [details]
Patch

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

>> Source/WTF/ChangeLog:13
>> +        deadlock.
> 
> It's not clear to me how this patch fixes the LLDB deadlock?  At some point, we will call setExceptionPort() for the signals we wish to handle.  How does having delayed the call to setExceptionPort() help after that it does get called?  Wouldn't LLDB also still deadlock then?  Or are you saying that you're only reducing the probability of deadlock here and not actually fixing it completely?  Can you comment and clarify this point in the ChangeLog.
> 
> Anyway, I think this patch is an improvement.

It doesn't fix the LLDB issue, I'll clarify that in the changelog. This patch is just to reduce the prevalence of the bug back to were it was. In a follow up patch I plan on only turning on the WASM memory when we lazily allocate the WebAssembly object.

>> Source/WTF/wtf/threads/Signals.cpp:243
>> +    kern_return_t result = thread_set_exception_ports(thread.machThread(), activeExceptions, handlers.exceptionPort, EXCEPTION_STATE | MACH_EXCEPTION_CODES, MACHINE_THREAD_STATE);
> 
> Replace "activeExceptions" with "activeExceptions & handlers.addedExceptions" here.

That's a good idea. I'll rename it to `handlers.registeredExecptions` though.

>> Source/WTF/wtf/threads/Signals.cpp:-295
>> -        Config::AssertNotFrozenScope assertScope;
> 
> Please restore this Config::AssertNotFrozenScope.

Done. I didn't mean for this version of the patch to be marked r? >.>

>> Source/WTF/wtf/threads/Signals.cpp:323
>> +        activeExceptions |= toMachMask(signal);
> 
> To ensure that we don't allow activating any handlers for signals that we didn't explicitly call addSignalHandler() for, let's do the following:
> 1. In SignalHandlers::add(), please add the following:
>         addedExceptions |= toMachMask(signal);
> 2. Here in activateSignalHandlersFor(), change this line to:
>         activeExceptions |= (toMachMask(signal) & handlers.addedExceptions);

I don't think we need to change this line because we will & with registeredExceptions.

>> Source/WTF/wtf/threads/Signals.h:112
>>      exception_mask_t activeExceptions;
> 
> Rename this "activeExceptions" field to to "addedExceptions".

Ditto on renaming.

[Keith Miller]

Ehhh, I decided against registeredExecptions in favor of addedExcepions.

[Keith Miller]

Created attachment 401910 [details]
Patch for landing

[EWS]

Committed r263045: <https://trac.webkit.org/changeset/263045>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401910 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64372103>