Bug 193085

Summary: Baseline version of get_by_id may corrupt metadata
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Patch none

Description Tadeu Zagallo 2019-01-02 12:22:40 PST
The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell` if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things:
1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by `CodeBlock::getOrAddArrayProfile`.
2) get_by_id's mode *is* ArrayLengt and we generate the array profiling code pointing to the ArrayProfile that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens, the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.
Comment 1 Tadeu Zagallo 2019-01-02 12:29:40 PST
<rdar://problem/23453006>
Comment 2 Tadeu Zagallo 2019-01-02 12:30:50 PST
Created attachment 358193 [details]
Patch
Comment 3 Mark Lam 2019-01-02 13:23:13 PST
Comment on attachment 358193 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:16
> +        The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell`
> +        if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry
> +        in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things:
> +        1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by
> +        `CodeBlock::getOrAddArrayProfile`.
> +        2) get_by_id's mode *is* ArrayLength and we generate the array profiling code pointing to the ArrayProfile
> +        that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens,
> +        the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.

I don't understand: I see that metadata.mode is set to GetByIdMode::ArrayLength in the LLInt slow_path_get_by_id.  From reading the code, the determination of metadata.mode is a function of the baseValue and the PropertySlot returned by get().  This means that metadata.mode is set at runtime, and can vary depending on runtime variables.  What's keeping it from changing again after it has been set once?

Your baseline code seems to be making an assumption about where to get the ArrayProfile address from.  I see this in emitArrayProfilingSiteForBytecodeIndexWithCell() which gets the ArrayProfile address from m_codeBlock->getOrAddArrayProfile(), which in turn determines the ArrayProfile address based on metadata.mode.  Since metadata.mode can change at runtime, the baseline JIT will be racing against the mutator thread.  The baseline JIT may compile with a GetByIdMode::ArrayLength type ArrayProfile and the mutator thread may change the metadata.mode to something other than GetByIdMode::ArrayLength right after, or vice versa.

The baseline JIT emitted runtime check (that you added below) comparing metadata.mode against GetByIdMode::ArrayLength will not protect against this.  Is there a reason why the GetById metadata can't have a static shape?  I think that that would be the ideal solution.
Comment 4 Tadeu Zagallo 2019-01-02 13:54:25 PST
Comment on attachment 358193 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.
> 
> I don't understand: I see that metadata.mode is set to GetByIdMode::ArrayLength in the LLInt slow_path_get_by_id.  From reading the code, the determination of metadata.mode is a function of the baseValue and the PropertySlot returned by get().  This means that metadata.mode is set at runtime, and can vary depending on runtime variables.  What's keeping it from changing again after it has been set once?
> 
> Your baseline code seems to be making an assumption about where to get the ArrayProfile address from.  I see this in emitArrayProfilingSiteForBytecodeIndexWithCell() which gets the ArrayProfile address from m_codeBlock->getOrAddArrayProfile(), which in turn determines the ArrayProfile address based on metadata.mode.  Since metadata.mode can change at runtime, the baseline JIT will be racing against the mutator thread.  The baseline JIT may compile with a GetByIdMode::ArrayLength type ArrayProfile and the mutator thread may change the metadata.mode to something other than GetByIdMode::ArrayLength right after, or vice versa.
> 
> The baseline JIT emitted runtime check (that you added below) comparing metadata.mode against GetByIdMode::ArrayLength will not protect against this.  Is there a reason why the GetById metadata can't have a static shape?  I think that that would be the ideal solution.

I could move the ArrayProfile out of the mode specific metadata, but that would add an extra 16 bytes to the get_by_id metadata, which would only be used for the ArrayLength mode... this seems a bit wasteful. I think I'd rather just use `emitArrayProfilingSiteWithCell` from `emit_op_get_by_id` and make sure we always refer to the ArrayProfile in the metadata. (And perhaps replace the conditional return in the `CodeBlock::getArrayProfile` with an assertion, not sure if that's correct though.)

Do you think that makes sense?
Comment 5 Tadeu Zagallo 2019-01-02 14:04:27 PST
Created attachment 358202 [details]
Patch
Comment 6 Tadeu Zagallo 2019-01-02 15:15:50 PST
Created attachment 358213 [details]
Patch
Comment 7 EWS Watchlist 2019-01-02 16:43:34 PST
Comment on attachment 358213 [details]
Patch

Attachment 358213 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10609586

New failing tests:
stress/v8-splay-strict.js.dfg-eager
v8-v6/v8-splay.js.dfg-eager
apiTests
Comment 8 EWS Watchlist 2019-01-02 17:07:58 PST
Comment on attachment 358213 [details]
Patch

Attachment 358213 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10609895

New failing tests:
js/dfg-inline-arguments-use-from-all-the-places-broken.html
Comment 9 EWS Watchlist 2019-01-02 17:08:00 PST
Created attachment 358225 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2019-01-02 17:19:34 PST
Comment on attachment 358213 [details]
Patch

Attachment 358213 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10609708

New failing tests:
js/dfg-inline-arguments-use-from-all-the-places-broken.html
Comment 11 EWS Watchlist 2019-01-02 17:19:36 PST
Created attachment 358228 [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 12 EWS Watchlist 2019-01-02 18:32:34 PST
Comment on attachment 358213 [details]
Patch

Attachment 358213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10610619

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-small.html
Comment 13 EWS Watchlist 2019-01-02 18:32:36 PST
Created attachment 358239 [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 14 Tadeu Zagallo 2019-01-03 05:31:05 PST
Created attachment 358251 [details]
Patch
Comment 15 Saam Barati 2019-01-04 09:09:12 PST
Comment on attachment 358251 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2019-01-04 11:52:33 PST
Comment on attachment 358251 [details]
Patch

Clearing flags on attachment: 358251

Committed r239626: <https://trac.webkit.org/changeset/239626>
Comment 17 WebKit Commit Bot 2019-01-04 11:52:35 PST
All reviewed patches have been landed.  Closing bug.