Bug 183863 - ScopedArguments should do poisoning and index masking
Summary: ScopedArguments should do poisoning and index masking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-21 12:20 PDT by Filip Pizlo
Modified: 2018-03-21 19:17 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (22.97 KB, patch)
2018-03-21 12:23 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it might be done (32.82 KB, patch)
2018-03-21 14:21 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (32.87 KB, patch)
2018-03-21 15:00 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (32.93 KB, patch)
2018-03-21 15:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (32.92 KB, patch)
2018-03-21 15:41 PDT, Filip Pizlo
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch for landing (33.20 KB, patch)
2018-03-21 18:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-03-21 12:20:56 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2018-03-21 12:23:23 PDT
Created attachment 336220 [details]
work in progress
Comment 2 Filip Pizlo 2018-03-21 14:21:54 PDT
Created attachment 336234 [details]
it might be done
Comment 3 EWS Watchlist 2018-03-21 14:24:01 PDT
Attachment 336234 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2018-03-21 15:00:50 PDT
Created attachment 336238 [details]
the patch
Comment 5 EWS Watchlist 2018-03-21 15:04:52 PDT
Attachment 336238 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Filip Pizlo 2018-03-21 15:29:06 PDT
Created attachment 336240 [details]
the patch
Comment 7 EWS Watchlist 2018-03-21 15:31:49 PDT
Attachment 336240 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Mark Lam 2018-03-21 15:32:04 PDT
Comment on attachment 336234 [details]
it might be done

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

r=me with issues addressed.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:407
> +        jit.loadPtr(
> +            CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()),

Shouldn't this be ScopedArguments::offsetOfStorage()?

> Source/JavaScriptCore/runtime/ScopedArguments.h:184
> +        static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue");
> +        return *bitwise_cast<StorageHeader*>(storage - 1);

You could have introduced a static method:
    static size_t storageHeaderSizeInJSValueCount() { return WTF::roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) / sizeof(WriteBarrier<Unknown>); }

... and use that instead assuming the StorageHeader takes 1 JSValue.  With that you can get rid of this assertion and make it such that we can add new fields to StorageHeader without breaking any code.  That said, we can always defer this till we need it in the future.
Comment 9 Filip Pizlo 2018-03-21 15:34:55 PDT
(In reply to Mark Lam from comment #8)
> Comment on attachment 336234 [details]
> it might be done
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336234&action=review
> 
> r=me with issues addressed.
> 
> > Source/JavaScriptCore/bytecode/AccessCase.cpp:407
> > +        jit.loadPtr(
> > +            CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()),
> 
> Shouldn't this be ScopedArguments::offsetOfStorage()?
> 
> > Source/JavaScriptCore/runtime/ScopedArguments.h:184
> > +        static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue");
> > +        return *bitwise_cast<StorageHeader*>(storage - 1);
> 
> You could have introduced a static method:
>     static size_t storageHeaderSizeInJSValueCount() { return
> WTF::
> roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) /
> sizeof(WriteBarrier<Unknown>); }
> 
> ... and use that instead assuming the StorageHeader takes 1 JSValue.  With
> that you can get rid of this assertion and make it such that we can add new
> fields to StorageHeader without breaking any code.  That said, we can always
> defer this till we need it in the future.

Lots of code assumes that StorageHeader is smaller than JSValue.
Comment 10 Filip Pizlo 2018-03-21 15:35:06 PDT
(In reply to Mark Lam from comment #8)
> Comment on attachment 336234 [details]
> it might be done
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336234&action=review
> 
> r=me with issues addressed.
> 
> > Source/JavaScriptCore/bytecode/AccessCase.cpp:407
> > +        jit.loadPtr(
> > +            CCallHelpers::Address(baseGPR, DirectArguments::offsetOfStorage()),
> 
> Shouldn't this be ScopedArguments::offsetOfStorage()?

Fixed!

> 
> > Source/JavaScriptCore/runtime/ScopedArguments.h:184
> > +        static_assert(sizeof(StorageHeader) <= sizeof(WriteBarrier<Unknown>), "StorageHeader needs to be no bigger than a JSValue");
> > +        return *bitwise_cast<StorageHeader*>(storage - 1);
> 
> You could have introduced a static method:
>     static size_t storageHeaderSizeInJSValueCount() { return
> WTF::
> roundUpToMultipleOf<sizeof(WriteBarrier<Unknown>)>(sizeof(StorageHeader)) /
> sizeof(WriteBarrier<Unknown>); }
> 
> ... and use that instead assuming the StorageHeader takes 1 JSValue.  With
> that you can get rid of this assertion and make it such that we can add new
> fields to StorageHeader without breaking any code.  That said, we can always
> defer this till we need it in the future.
Comment 11 Filip Pizlo 2018-03-21 15:41:51 PDT
Created attachment 336242 [details]
patch for landing

Addressed Mark's feedback
Comment 12 EWS Watchlist 2018-03-21 15:51:53 PDT
Attachment 336242 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 EWS Watchlist 2018-03-21 17:09:46 PDT
Comment on attachment 336242 [details]
patch for landing

Attachment 336242 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7057246

New failing tests:
microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager
microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit
microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-validate-phases
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-small-pool
microbenchmarks/scoped-arguments-possibly-overridden-length.js.dfg-eager-no-cjit-validate
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-eager-no-cjit-b3o1
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-b3o1
microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-ftl
microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-llint
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-no-inline-validate
microbenchmarks/scoped-arguments-possibly-overridden-length.js.default
microbenchmarks/scoped-arguments-possibly-overridden-length.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/scoped-arguments-possibly-overridden-length.js.no-cjit-collect-continuously
Comment 14 Filip Pizlo 2018-03-21 18:38:43 PDT
Created attachment 336254 [details]
patch for landing
Comment 15 EWS Watchlist 2018-03-21 18:40:50 PDT
Attachment 336254 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ScopedArguments.cpp:151:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Filip Pizlo 2018-03-21 19:16:04 PDT
Landed in https://trac.webkit.org/changeset/229842/webkit
Comment 17 Radar WebKit Bug Importer 2018-03-21 19:17:19 PDT
<rdar://problem/38734279>