Bug 183863 – ScopedArguments should do poisoning and index masking

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 336220 [details]
work in progress

[Filip Pizlo]

Created attachment 336234 [details]
it might be done

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 336238 [details]
the patch

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 336240 [details]
the patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 336242 [details]
patch for landing

Addressed Mark's feedback

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Filip Pizlo]

Created attachment 336254 [details]
patch for landing

[EWS Watchlist]

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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/229842/webkit

[Radar WebKit Bug Importer]

<rdar://problem/38734279>