Bug 182155 - Apply poisoning to TypedArray vector pointers.
Summary: Apply poisoning to TypedArray vector pointers.
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:
Blocks:
 
Reported: 2018-01-25 15:27 PST by Mark Lam
Modified: 2018-01-31 14:16 PST (History)
11 users (show)

See Also:


Attachments
work in progress for EWS testing. (39.45 KB, patch)
2018-01-25 16:09 PST, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (39.47 KB, patch)
2018-01-25 16:20 PST, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (40.92 KB, patch)
2018-01-25 22:11 PST, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.22 MB, application/zip)
2018-01-25 23:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.54 MB, application/zip)
2018-01-25 23:29 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.14 MB, application/zip)
2018-01-26 01:51 PST, EWS Watchlist
no flags Details
work in progress for EWS testing. (40.86 KB, patch)
2018-01-26 11:12 PST, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.28 MB, application/zip)
2018-01-26 12:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.14 MB, application/zip)
2018-01-26 12:51 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.02 MB, application/zip)
2018-01-26 12:53 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.92 MB, application/zip)
2018-01-26 16:50 PST, EWS Watchlist
no flags Details
work in progress for EWS testing. (43.11 KB, patch)
2018-01-29 14:55 PST, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.17 MB, application/zip)
2018-01-29 16:08 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.68 MB, application/zip)
2018-01-29 16:17 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.25 MB, application/zip)
2018-01-29 16:32 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (2.93 MB, application/zip)
2018-01-29 16:42 PST, EWS Watchlist
no flags Details
work in progress for EWS testing. (45.15 KB, patch)
2018-01-29 23:15 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (48.54 KB, patch)
2018-01-30 12:13 PST, Mark Lam
jfbastien: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (49.95 KB, patch)
2018-01-30 20:12 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing. (49.96 KB, patch)
2018-01-30 20:26 PST, 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 2018-01-25 15:27:35 PST
<rdar://problem/36286266>
Comment 1 Mark Lam 2018-01-25 16:09:31 PST
Created attachment 332330 [details]
work in progress for EWS testing.
Comment 2 EWS Watchlist 2018-01-25 16:12:05 PST
Attachment 332330 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2018-01-25 16:20:43 PST
Created attachment 332332 [details]
work in progress for EWS testing.
Comment 4 EWS Watchlist 2018-01-25 16:22:29 PST
Attachment 332332 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mark Lam 2018-01-25 16:39:08 PST
Comment on attachment 332332 [details]
work in progress for EWS testing.

I think I've found bugs that I need to work on.  No need to continue testing.
Comment 6 Mark Lam 2018-01-25 22:11:09 PST
Created attachment 332355 [details]
work in progress for EWS testing.
Comment 7 EWS Watchlist 2018-01-25 22:12:36 PST
Attachment 332355 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 EWS Watchlist 2018-01-25 23:25:21 PST
Comment on attachment 332355 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 9 EWS Watchlist 2018-01-25 23:25:22 PST
Created attachment 332356 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-01-25 23:29:35 PST
Comment on attachment 332355 [details]
work in progress for EWS testing.

Attachment 332355 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6218730

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 11 EWS Watchlist 2018-01-25 23:29:37 PST
Created attachment 332357 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-01-26 01:51:43 PST
Comment on attachment 332355 [details]
work in progress for EWS testing.

Attachment 332355 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6219345

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 13 EWS Watchlist 2018-01-26 01:51:45 PST
Created attachment 332361 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 14 Mark Lam 2018-01-26 11:12:44 PST
Created attachment 332389 [details]
work in progress for EWS testing.
Comment 15 EWS Watchlist 2018-01-26 11:15:44 PST
Attachment 332389 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 EWS Watchlist 2018-01-26 12:25:05 PST
Comment on attachment 332389 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 17 EWS Watchlist 2018-01-26 12:25:06 PST
Created attachment 332400 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 EWS Watchlist 2018-01-26 12:51:48 PST
Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6223760

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 19 EWS Watchlist 2018-01-26 12:51:49 PST
Created attachment 332404 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 20 EWS Watchlist 2018-01-26 12:53:48 PST
Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6223948

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 21 EWS Watchlist 2018-01-26 12:53:50 PST
Created attachment 332405 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 22 EWS Watchlist 2018-01-26 16:48:43 PST
Comment on attachment 332389 [details]
work in progress for EWS testing.

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

New failing tests:
stress/array-message-passing.js.ftl-eager-no-cjit
Comment 23 EWS Watchlist 2018-01-26 16:50:40 PST
Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6226084

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 24 EWS Watchlist 2018-01-26 16:50:42 PST
Created attachment 332431 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 25 Mark Lam 2018-01-29 14:55:45 PST
Created attachment 332584 [details]
work in progress for EWS testing.
Comment 26 EWS Watchlist 2018-01-29 14:57:49 PST
Attachment 332584 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 EWS Watchlist 2018-01-29 16:08:54 PST
Comment on attachment 332584 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 28 EWS Watchlist 2018-01-29 16:08:55 PST
Created attachment 332595 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 29 EWS Watchlist 2018-01-29 16:17:53 PST
Comment on attachment 332584 [details]
work in progress for EWS testing.

Attachment 332584 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6253901

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 30 EWS Watchlist 2018-01-29 16:17:54 PST
Created attachment 332596 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 31 EWS Watchlist 2018-01-29 16:32:45 PST
Comment on attachment 332584 [details]
work in progress for EWS testing.

Attachment 332584 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6253855

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 32 EWS Watchlist 2018-01-29 16:32:46 PST
Created attachment 332598 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 33 EWS Watchlist 2018-01-29 16:42:06 PST
Comment on attachment 332584 [details]
work in progress for EWS testing.

Attachment 332584 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6253944

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 34 EWS Watchlist 2018-01-29 16:42:08 PST
Created attachment 332601 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 35 Mark Lam 2018-01-29 23:15:54 PST
Created attachment 332630 [details]
work in progress for EWS testing.
Comment 36 EWS Watchlist 2018-01-29 23:17:46 PST
Attachment 332630 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Mark Lam 2018-01-30 12:13:47 PST
Created attachment 332680 [details]
proposed patch.
Comment 38 EWS Watchlist 2018-01-30 12:16:05 PST
Attachment 332680 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Mark Lam 2018-01-30 12:28:19 PST
Comment on attachment 332680 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:5
> +        Apply poisoning to TypedArray vector pointers.
> +        https://bugs.webkit.org/show_bug.cgi?id=182155
> +        <rdar://problem/36286266>

I should add a comment about how I poison the vector pointers.  Basically, ...

The TypeArray's vector pointer is now poisoned.  The poison value is chosen based on a TypeArray's jsType.  The JSType must be between FirstTypedArrayType and LastTypedArrayType.  At runtime, we enforce that the index is well-behaved by masking it against TypedArrayPoisonIndexMask.  TypedArrayPoisonIndexMask (16) is the number of TypedArray types (10) rounded up to the next power of 2.  Accordingly, we reserve an array of TypedArrayPoisonIndexMask poisons so that we can use index masking on the index.
Comment 40 EWS Watchlist 2018-01-30 13:21:42 PST
Comment on attachment 332680 [details]
proposed patch.

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

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit
Comment 41 Mark Lam 2018-01-30 14:47:18 PST
(In reply to Build Bot from comment #40)
> Comment on attachment 332680 [details]
> proposed patch.
> 
> Attachment 332680 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6264300
> 
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> cjit

Strange, but I can't seem to reproduce this locally.  Still trying.
Comment 42 JF Bastien 2018-01-30 15:44:57 PST
Comment on attachment 332680 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6388
> +    GPRReg indexGPR = index.gpr();

Assert that poison/index aren't the same as base/vector?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6408
> +    m_jit.xorPtr(poisonGPR, vectorGPR);

Blow away the poisonGPR content after?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3536
> +        poisonedVector = m_out.bitXor(poisonedVector, poison);

Zero out the poison Lvalue after?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:402
> +        xorp scratch, dest

No need to zero out scratch if GIGACAGE_ENABLED and not C_LOOP because it'll be clobbered. I guess we don't care otherwise?

> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:34
> +template<typename Poison, typename T> struct PoisonedPtrTraits;

Include Forwards.h instead.

> Source/JavaScriptCore/runtime/JSArrayBufferView.h:226
> +    PoisonedCagedBarrierPtr<Poison, Gigacage::Primitive, void> m_poisonedVector;

🤯 a vector of poisons, held in a poisoned caged barrier pointer 🤯

> Source/WTF/wtf/CagedPtr.h:38
> +    explicit CagedPtr(T* ptr = nullptr)

Do you want to add a nullptr_t ctor since this one is now explicit (if someone doesn't want to rely on default)?

> Source/WTF/wtf/CagedPtr.h:98
> +    explicit CagedPtr(void* ptr = nullptr)

Ditto.
Comment 43 Mark Lam 2018-01-30 19:51:03 PST
Comment on attachment 332680 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6388
>> +    GPRReg indexGPR = index.gpr();
> 
> Assert that poison/index aren't the same as base/vector?

Don't need an assert here.  All 4 values are local GPRTemporarys just instantiated above.  Hence, they are guaranteed to be mutually exclusive with each other and with base.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6408
>> +    m_jit.xorPtr(poisonGPR, vectorGPR);
> 
> Blow away the poisonGPR content after?

Fixed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:402
>> +        xorp scratch, dest
> 
> No need to zero out scratch if GIGACAGE_ENABLED and not C_LOOP because it'll be clobbered. I guess we don't care otherwise?

Fixed.

>> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:34
>> +template<typename Poison, typename T> struct PoisonedPtrTraits;
> 
> Include Forwards.h instead.

I'll fix this in a subsequent patch.  Currently, no one #include Forward.h for this decl.  We can change that, but it shouldn't be in just this one place.

>> Source/WTF/wtf/CagedPtr.h:38
>> +    explicit CagedPtr(T* ptr = nullptr)
> 
> Do you want to add a nullptr_t ctor since this one is now explicit (if someone doesn't want to rely on default)?

Fixed.

>> Source/WTF/wtf/CagedPtr.h:98
>> +    explicit CagedPtr(void* ptr = nullptr)
> 
> Ditto.

Fixed.
Comment 44 Mark Lam 2018-01-30 20:12:05 PST
Created attachment 332737 [details]
Patch for landing.
Comment 45 EWS Watchlist 2018-01-30 20:13:34 PST
Attachment 332737 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CagedPtr.h:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:101:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:102:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Mark Lam 2018-01-30 20:24:37 PST
(In reply to Build Bot from comment #40)
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> cjit

1. I can't reproduce this failure locally.
2. The failing test doesn't even use TypedArrays.

So, I'm quite sure this is not related to my patch.
Comment 47 Mark Lam 2018-01-30 20:26:20 PST
Created attachment 332739 [details]
patch for landing.
Comment 48 EWS Watchlist 2018-01-30 20:28:06 PST
Attachment 332739 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CagedPtr.h:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:101:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:102:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Mark Lam 2018-01-30 21:24:47 PST
Landed in r227874: <http://trac.webkit.org/r227874>.
Comment 50 Mark Lam 2018-01-31 14:16:43 PST
Build fix for CLoop landed in r227929: <http://trac.webkit.org/r227929>.