1431173 - Use index masking for more bounds checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 2

[Jan de Mooij [:jandem]]

(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 3

[Nicolas B. Pierron [:nbp]]

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".

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e0e51f4a5b
Use Spectre index masking for more bounds checked loads. r=nbp

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c3e0e51f4a5b

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8943344 [details] [diff] [review]
Patch

Spectre-related fix. Taking for 59b8.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8d51c323a86

Comment 9

[Andrei Vaida [:avaida]]

(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.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Do we want a release note for this? (sorry for spam if one is already planned / in place)