Use index masking for more bounds checks

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed, firefox60 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
Posted patch PatchSplinter Review
This adds MacroAssembler::boundsCheck32ForLoad and uses it in Ion and IC code.

For this to work I also had to refactor the MacroAssembler index masking code: we now have helper methods to compute the mask, and then the callers can either store the result in the index register or the output register.
Attachment #8943344 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8943344 [details] [diff] [review]
Patch

Review of attachment 8943344 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drive-by nit (we probably can use the helpers here for wasm too).

::: js/src/jit/MacroAssembler.cpp
@@ +3435,2 @@
>      // mask := ((index - length) & ~index) >> 31
>      // output := index & mask

nit: unless i'm missing something, this last line is now done by the callers?
Assignee

Comment 2

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> >      // mask := ((index - length) & ~index) >> 31
> >      // output := index & mask
> 
> nit: unless i'm missing something, this last line is now done by the callers?

Oh, good catch, thanks! Yeah I should remove that line here.
Comment on attachment 8943344 [details] [diff] [review]
Patch

Review of attachment 8943344 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +10972,5 @@
>  
>      // Load the length.
>      Register scratch = out.scratchReg();
> +    Register scratch2 = ToRegister(lir->temp());
> +    Register key = ToRegister(lir->index());

follow-up nit: rename key to index.

@@ +10992,5 @@
> +    masm.jump(&done);
> +
> +    masm.bind(&outOfBounds);
> +    masm.moveValue(UndefinedValue(), out);
> +    masm.jump(&done);

nit: This last jump should not be necessary, as bailoutFrom create an OOL code path for all jump to "fail".
Attachment #8943344 - Flags: review?(nicolas.b.pierron) → review+

Comment 4

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e0e51f4a5b
Use Spectre index masking for more bounds checked loads. r=nbp

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3e0e51f4a5b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee

Comment 6

a year ago
Comment on attachment 8943344 [details] [diff] [review]
Patch

We would like to uplift a number of patches for Spectre mitigations to beta.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Spectre security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: I'll request approval on them.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Has been fuzzed/tested on Nightly for a while.
[String changes made/needed]: None.
Attachment #8943344 - Flags: approval-mozilla-beta?
Comment on attachment 8943344 [details] [diff] [review]
Patch

Spectre-related fix. Taking for 59b8.
Attachment #8943344 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jan de Mooij [:jandem] from comment #6)
> [Feature/Bug causing the regression]: N/A
> [User impact if declined]: Spectre security issues.
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Do we want a release note for this? (sorry for spam if one is already planned / in place)
You need to log in before you can comment on or make changes to this bug.