Summary: | ASSERTION FAILED: !m_needExceptionCheck in CloneSerializer::serialize with postMessage({g:42}) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | Bindings | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | alecflett, beidson, bfulgham, cdumez, cgarcia, ews-feeder, ews-watchlist, fred.wang, gpoo, jsbell, mark.lam, product-security, rbuis, sam, svillar, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-03-26 01:58:28 PDT
I was able to reproduce this with debug build of WebKitTestRunner at r273504. Note that you need to specify __XPC_JSC_validateExceptionChecks=1 as an environment variable on macOS port. i.e. enable JSC's validateExceptionChecks option. Created attachment 425618 [details]
Patch (proof-of-concept)
So I've been debugging this with the help of Caio, and he thinks CloneSerializer::serialize should check potential exceptions thrown by getOwnPropertyNames as well.
Mimicing the current approach with shouldTerminate() does not work here, because IIUC it will just rethrow the exception immediately when we create the second throw scope. Also in general, Caio thinks there could be an issue with that approach because of the Proxy object. He also wants to think and check a bit more what would be the correct approach here. I'll let him explain things better...
Anyway, here is a proof-of-concept patch that fixes the crash, so that people can comment.
Comment on attachment 425618 [details] Patch (proof-of-concept) View in context: https://bugs.webkit.org/attachment.cgi?id=425618&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1841 > + if (scope.exception()) Can we also fix ArrayStartVisitMember and SetDataStartVisitEntry? Created attachment 425736 [details]
Patch
Comment on attachment 425618 [details] Patch (proof-of-concept) View in context: https://bugs.webkit.org/attachment.cgi?id=425618&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1841 >> + if (scope.exception()) > > Can we also fix ArrayStartVisitMember and SetDataStartVisitEntry? Done. Comment on attachment 425736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425736&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Same here, I don't know whether or not I should include the test. Comment on attachment 425736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425736&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1791 > + if (scope.exception()) Let's put `UNLIKELY()` to these exception checks. Comment on attachment 425736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425736&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1863 > inValue = getProperty(vm, object, properties[index]); > - if (shouldTerminate()) > + if (scope.exception()) > return SerializationReturnCode::ExistingExceptionError; > > if (!inValue) { This is the only interesting place when considering about security related thing (whether inValue is valid or not if we ignore exception). But shouldTerminate checked exception anyway, so this is OK. Created attachment 425841 [details]
Patch
Thanks for the explanation and review. here is a new version with a test.
Comment on attachment 425841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425841&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795 > indexStack.append(0); Let's insert error check after getDirectIndex (in L1804). Comment on attachment 425841 [details]
Patch
The other part looks good to me.
Created attachment 425852 [details]
Patch for landing
Comment on attachment 425841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425841&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795 >> indexStack.append(0); > > Let's insert error check after getDirectIndex (in L1804). Done (I assumed you meant adding a new one, not moving that one... similar to the case of getProperty below) Comment on attachment 425841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425841&action=review >>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1795 >>> indexStack.append(0); >> >> Let's insert error check after getDirectIndex (in L1804). > > Done (I assumed you meant adding a new one, not moving that one... similar to the case of getProperty below) Yes, adding a new one :) Committed r275882 (236447@main): <https://commits.webkit.org/236447@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425852 [details]. |