Bug 193085 – Baseline version of get_by_id may corrupt metadata

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

<rdar://problem/23453006>

[Tadeu Zagallo]

Created attachment 358193 [details]
Patch

[Mark Lam]

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.

[Tadeu Zagallo]

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?

[Tadeu Zagallo]

Created attachment 358202 [details]
Patch

[Tadeu Zagallo]

Created attachment 358213 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 358251 [details]
Patch

[Saam Barati]

Comment on attachment 358251 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 358251 [details]
Patch

Clearing flags on attachment: 358251

Committed r239626: <https://trac.webkit.org/changeset/239626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.