Bug 214624 - Disallow VM entry when doing a VMInquiry.
Summary: Disallow VM entry when doing a VMInquiry.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 214539
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-21 19:14 PDT by Mark Lam
Modified: 2020-07-26 12:06 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (46.44 KB, patch)
2020-07-21 19:48 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (63.55 KB, patch)
2020-07-21 23:19 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (67.20 KB, patch)
2020-07-21 23:28 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (65.33 KB, patch)
2020-07-22 15:25 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-07-21 19:14:35 PDT
We can now use the DisallowVMEntry scope to enforce this.
Comment 1 Radar WebKit Bug Importer 2020-07-21 19:16:45 PDT
<rdar://problem/65915314>
Comment 2 Mark Lam 2020-07-21 19:48:18 PDT
Created attachment 404892 [details]
proposed patch.
Comment 3 Mark Lam 2020-07-21 23:19:21 PDT
Created attachment 404901 [details]
proposed patch.
Comment 4 Mark Lam 2020-07-21 23:28:16 PDT
Created attachment 404902 [details]
proposed patch.
Comment 5 Saam Barati 2020-07-22 11:36:38 PDT
Comment on attachment 404902 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:27
> +           do entail entering the VM.  In such cases, we need to reset the PropertySlot's

do entail => entails

> Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp:310
> +        PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry, &lexicalGlobalObject->vm() };
> +        bool found = JSObject::getOwnPropertySlot(thisObject, lexicalGlobalObject, propertyName, slot);
> +        slot.disallowVMEntry.reset();

for a lot of these that just require the bool result of calling getOwnPropertySlot, if you just passed the slot in as a constructed thing in the function call, you wouldn't need the reset dance, since it's destroyed post call. 

(I haven't vetted all call sites, but a lot could do this, and would be cleaner for it)
Comment 6 Mark Lam 2020-07-22 13:06:54 PDT
Comment on attachment 404902 [details]
proposed patch.

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp:310
>> +        slot.disallowVMEntry.reset();
> 
> for a lot of these that just require the bool result of calling getOwnPropertySlot, if you just passed the slot in as a constructed thing in the function call, you wouldn't need the reset dance, since it's destroyed post call. 
> 
> (I haven't vetted all call sites, but a lot could do this, and would be cleaner for it)

I'll add a helper in JSObject to handle this.
Comment 7 Mark Lam 2020-07-22 15:25:47 PDT
Created attachment 404984 [details]
patch for landing.
Comment 8 Mark Lam 2020-07-22 16:57:55 PDT
Comment on attachment 404984 [details]
patch for landing.

After thinking about this more, I'm going to opt for not adding the helper functions.  The reason is because it is not possible to use the helpers in all places.  For example,
1. some places needs to use the PropertySlot after the call to "get".
2. some places want to call "get" via the methodTable.  Hence, the helpers will not be helpful there as they only dispatch to the JSObject methods.

Adopting the helpers will therefore result in having more than one way to do VMInquiries, and can make the code more confusing.  So, I'm going to forego the helpers in the interest of keeping the interface consistent.

Instantiating the PropertySlot directly as the argument for the "get" function is also not an option.  This is because this "get" functions require a PropertySlot&.  C++ does not let us pass a reference to this unnamed instantiated PropertySlot.

I'll land the original patch with the ChangeLog typo fix.  Thanks.
Comment 9 Mark Lam 2020-07-22 17:09:57 PDT
Thanks for the review.  Landed in r264736: <http://trac.webkit.org/r264736>.
Comment 10 Michael Catanzaro 2020-07-26 12:06:14 PDT
GCC found a problem: bug #214809