Bug 234447 - null ptr deref while trying to access DeferredPromise::promise()
Summary: null ptr deref while trying to access DeferredPromise::promise()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gabriel Nava Marino
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-17 15:22 PST by Gabriel Nava Marino
Modified: 2022-01-13 13:54 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.20 KB, patch)
2021-12-17 15:43 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2022-01-11 10:43 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2022-01-11 11:05 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2022-01-11 14:39 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (8.61 KB, patch)
2022-01-12 09:57 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2021-12-17 15:22:03 PST
This occurs after a given DeferredPromise has had its context destroyed in Document::~Document.
Comment 1 Gabriel Nava Marino 2021-12-17 15:43:01 PST
Created attachment 447487 [details]
Patch
Comment 2 Gabriel Nava Marino 2021-12-17 15:44:17 PST
<rdar://problem/83069008>
Comment 3 Chris Dumez 2021-12-17 15:56:18 PST
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 4 Chris Dumez 2021-12-17 15:58:44 PST
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?
Comment 5 Gabriel Nava Marino 2022-01-11 10:43:47 PST
Created attachment 448855 [details]
Patch
Comment 6 Gabriel Nava Marino 2022-01-11 10:44:47 PST
(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.
Comment 7 Gabriel Nava Marino 2022-01-11 11:05:25 PST
Created attachment 448858 [details]
Patch
Comment 8 Darin Adler 2022-01-11 11:14:26 PST
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?
Comment 9 Gabriel Nava Marino 2022-01-11 11:15:54 PST
(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.
Comment 10 Darin Adler 2022-01-11 11:16:51 PST
I see now: sorry for not reviewing the comments before reviewing.
Comment 11 Gabriel Nava Marino 2022-01-11 12:00:06 PST
(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!
Comment 12 Gabriel Nava Marino 2022-01-11 14:39:43 PST
Created attachment 448880 [details]
Patch
Comment 13 Gabriel Nava Marino 2022-01-12 09:57:49 PST
Created attachment 448959 [details]
Patch
Comment 14 Gabriel Nava Marino 2022-01-12 10:00:08 PST
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 15 Darin Adler 2022-01-13 13:38:29 PST
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.
Comment 16 EWS 2022-01-13 13:54:22 PST
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].