WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
193085
Baseline version of get_by_id may corrupt metadata
https://bugs.webkit.org/show_bug.cgi?id=193085
Summary
Baseline version of get_by_id may corrupt metadata
Tadeu Zagallo
Reported
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.
Attachments
Patch
(4.29 KB, patch)
2019-01-02 12:30 PST
,
Tadeu Zagallo
no flags
Details
Formatted Diff
Diff
Patch
(4.39 KB, patch)
2019-01-02 14:04 PST
,
Tadeu Zagallo
no flags
Details
Formatted Diff
Diff
Patch
(11.61 KB, patch)
2019-01-02 15:15 PST
,
Tadeu Zagallo
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews100 for mac-sierra
(2.47 MB, application/zip)
2019-01-02 17:08 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews113 for mac-sierra
(2.00 MB, application/zip)
2019-01-02 17:19 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews104 for mac-sierra-wk2
(3.25 MB, application/zip)
2019-01-02 18:32 PST
,
EWS Watchlist
no flags
Details
Patch
(13.94 KB, patch)
2019-01-03 05:31 PST
,
Tadeu Zagallo
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Tadeu Zagallo
Comment 1
2019-01-02 12:29:40 PST
<
rdar://problem/23453006
>
Tadeu Zagallo
Comment 2
2019-01-02 12:30:50 PST
Created
attachment 358193
[details]
Patch
Mark Lam
Comment 3
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.
Tadeu Zagallo
Comment 4
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?
Tadeu Zagallo
Comment 5
2019-01-02 14:04:27 PST
Created
attachment 358202
[details]
Patch
Tadeu Zagallo
Comment 6
2019-01-02 15:15:50 PST
Created
attachment 358213
[details]
Patch
EWS Watchlist
Comment 7
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
EWS Watchlist
Comment 8
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
EWS Watchlist
Comment 9
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
EWS Watchlist
Comment 10
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
EWS Watchlist
Comment 11
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
EWS Watchlist
Comment 12
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
EWS Watchlist
Comment 13
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
Tadeu Zagallo
Comment 14
2019-01-03 05:31:05 PST
Created
attachment 358251
[details]
Patch
Saam Barati
Comment 15
2019-01-04 09:09:12 PST
Comment on
attachment 358251
[details]
Patch r=me
WebKit Commit Bot
Comment 16
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
>
WebKit Commit Bot
Comment 17
2019-01-04 11:52:35 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug