Summary: | null ptr deref while trying to access DeferredPromise::promise() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gabriel Nava Marino <gnavamarino> | ||||||||||||
Component: | Bindings | Assignee: | Gabriel Nava Marino <gnavamarino> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, darin, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Gabriel Nava Marino
2021-12-17 15:22:03 PST
Created attachment 447487 [details]
Patch
Comment on attachment 447487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447487&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47 > + if (shouldIgnoreRequestToFulfill()) Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach. Comment on attachment 447487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447487&action=review Seems like this needs a test to land? >> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47 >> + if (shouldIgnoreRequestToFulfill()) > > Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach. Maybe you can use the isEmpty() check instead? Created attachment 448855 [details]
Patch
(In reply to Chris Dumez from comment #4) > Comment on attachment 447487 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447487&action=review > > Seems like this needs a test to land? > > >> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47 > >> + if (shouldIgnoreRequestToFulfill()) > > > > Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach. > > Maybe you can use the isEmpty() check instead? Thank you for the feedback, I have updated the change to use isEmpty() instead. I have also attached a test case which catches the issue. Created attachment 448858 [details]
Patch
Comment on attachment 448858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448858&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47 > + if (isEmpty()) Other functions in this class use shouldIgnoreRequestToFulfill rather than calling isEmpty directly. Did you consider that? (In reply to Darin Adler from comment #8) > Comment on attachment 448858 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448858&action=review > > > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47 > > + if (isEmpty()) > > Other functions in this class use shouldIgnoreRequestToFulfill rather than > calling isEmpty directly. Did you consider that? Yes, the original proposal used shouldIgnoreRequestToFulfill(), but I have updated to isEmpty() per https://bugs.webkit.org/show_bug.cgi?id=234447#c3. I see now: sorry for not reviewing the comments before reviewing. (In reply to Darin Adler from comment #10) > I see now: sorry for not reviewing the comments before reviewing. No problem. Thank you for the review! Created attachment 448880 [details]
Patch
Created attachment 448959 [details]
Patch
Update TestExpectations to skip the test on iOS (as well as windows) as pointer-lock, which this test relies on, is not supported there. Comment on attachment 448959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448959&action=review > LayoutTests/platform/ios/TestExpectations:181 > +fast/js-promise/js-promise-invalid-context-access.html [ Skip ] Seems really unfortunate that this test depends on pointer-lock. Committed r287993 (246020@main): <https://commits.webkit.org/246020@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448959 [details]. |