Mach exception handler could see uninitialized handler
Created attachment 362221 [details] Patch
<rdar://problem/47629892>
Comment on attachment 362221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362221&action=review Needs a regression test. Should be easy enough to send yourself a signal in an API test. I'm curious: Why eagerly initialize instead of just checking for null? > Source/WTF/ChangeLog:12 > + type, say bad access, we know how to handler but did not register handler -> handle
(In reply to Geoffrey Garen from comment #3) > Comment on attachment 362221 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362221&action=review > > Needs a regression test. Should be easy enough to send yourself a signal in > an API test. > > I'm curious: Why eagerly initialize instead of just checking for null? How would we test for this other than to crash the API test? This only happens if the process was about to crash anyway. This bug just steals someone else's thunder. LazyNeverDestroyed doesn't have a ! operator. I suppose we could add one though. It also just seemed like we would be less likely to have a bug like this in the future for the cost of 2 extra words. > > > Source/WTF/ChangeLog:12 > > + type, say bad access, we know how to handler but did not register > > handler -> handle Fixed.
Comment on attachment 362221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362221&action=review > Source/WTF/ChangeLog:16 > + This patch makes it so that we intialize all know handler bags Know => Known > Source/WTF/wtf/threads/Signals.cpp:260 > std::call_once(initializeOnceFlags[static_cast<size_t>(signal)], [&] { Shouldn’t we technically do this before starting the Mach exception handler thread?
Comment on attachment 362221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362221&action=review >>> Source/WTF/ChangeLog:12 >>> + type, say bad access, we know how to handler but did not register >> >> handler -> handle > > Fixed. “handle” => “handle it, but”
Comment on attachment 362221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362221&action=review > Source/WTF/ChangeLog:9 > + If we register a mach exception handler for some exception type, I now understand the bug and agree with your fix. I agree with Geoff that we should add a test for this.
> > Needs a regression test. Should be easy enough to send yourself a signal in > > an API test. > > How would we test for this other than to crash the API test? Can the API test install a signal handler and then send a user signal? Can the API test install a mach exception handler and then do an invalid memory read? Can catch_mach_exception_raise_state support a global override for testing that causes it to return KERN_SUCCESS even if didHandle is false?
Sorry, this is totally wrong. I forgot that there is a mask that controls which exceptions are derived to your dispatch queue. We would never see a signal for an exception we did not already initialize. That said, I'm ok making a code quality change here if people want that.