Bug 195791 - [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
Summary: [JSC] Retain PrivateName of Symbol before passing it to operations potentiall...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 20:36 PDT by Yusuke Suzuki
Modified: 2019-03-19 21:30 PDT (History)
9 users (show)

See Also:

Attachments
Patch (23.36 KB, patch)
2019-03-14 21:08 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (23.55 KB, patch)
2019-03-14 22:06 PDT, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-03-14 20:36:28 PDT 
[JSC] Do not pass Symbol::privateName() directly to PropertyName without ref-ing it
Comment 1 Yusuke Suzuki 2019-03-14 20:38:15 PDT 
<rdar://problem/48806130>
Comment 2 Yusuke Suzuki 2019-03-14 21:08:01 PDT 
Created attachment 364768 [details]
Patch
Comment 3 Yusuke Suzuki 2019-03-14 21:08:03 PDT 
<rdar://problem/48806130>
Comment 4 Yusuke Suzuki 2019-03-14 22:06:43 PDT 
Created attachment 364770 [details]
Patch
Comment 5 Mark Lam 2019-03-14 22:36:12 PDT 
Comment on attachment 364770 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:19
> +        PropertyName can be accidentally destroyed in the middle of putByVal operation. We should retain PrivateName

/middle of putByVal/middle of the putByVal/

> Source/JavaScriptCore/ChangeLog:37
> +        3. We audit similar functions `toPropertyKey(exec)` and `toIdentifier(exec)` necessary exception checks.

/necessary exception checks/for needed but missing exception checks/.

> Source/JavaScriptCore/runtime/JSONObject.cpp:250
> +                    auto propertyName = name.toString(exec)->toIdentifier(exec);

name.toString() can also throw an exception because it can allocate a string from a name that is a number.  So, you'll need another exception check before the toIdentifier() conversion.
Comment 6 Yusuke Suzuki 2019-03-14 22:38:16 PDT 
Comment on attachment 364770 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        PropertyName can be accidentally destroyed in the middle of putByVal operation. We should retain PrivateName
> 
> /middle of putByVal/middle of the putByVal/

Fixed.

>> Source/JavaScriptCore/ChangeLog:37
>> +        3. We audit similar functions `toPropertyKey(exec)` and `toIdentifier(exec)` necessary exception checks.
> 
> /necessary exception checks/for needed but missing exception checks/.

Fixed.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:250
>> +                    auto propertyName = name.toString(exec)->toIdentifier(exec);
> 
> name.toString() can also throw an exception because it can allocate a string from a name that is a number.  So, you'll need another exception check before the toIdentifier() conversion.

Nice catch. Fixed.
Comment 7 Yusuke Suzuki 2019-03-14 22:56:32 PDT 
Committed r242991: <https://trac.webkit.org/changeset/242991>
Comment 8 Robin Morisset 2019-03-15 08:16:01 PDT 
Comment on attachment 364770 [details]
Patch

LGTM too. Thanks a lot for solving this bug.
Comment 9 Mark Lam 2019-03-15 10:14:56 PDT 
Added a missing exception check in r242999: <http://trac.webkit.org/r242999>.
Comment 10 Yusuke Suzuki 2019-03-19 21:30:42 PDT 
Committed r243191: <https://trac.webkit.org/changeset/243191>