1324810 - Ion bug with RegExp{Prototype,Instance}OptimizableRaw and the exception handler

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

Attachment: Trigger crash

decoder found some impossible to reproduce crashes under HandleException. I did some code auditing and I found the bug.

To reproduce, apply the patch, then run |jit_test.py --ion| and you should get a number of crashes.

Ion calls RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw and if they return false, it jumps to masm.exceptionLabel(). The problem is that the stack doesn't have an exit frame at that point so the exception handler sees a bogus stack.

The patch just makes us always fail there and adds a JS_GC call to HandleException to trigger the crash more reliably.

The best fix is probably to make these functions infallible - if we hit an OOM in this code we could just recover from it and return false to treat it as non-optimizable.

Comment 1

[Jan de Mooij [:jandem]]

This code was added in November 2015 / January 2016. I think there were some changes to this code recently, maybe it made this code fallible and it used to be infallible? arai, do you happen to know? Else I can take a look at this later.

Comment 2

[Jan de Mooij [:jandem]]

It's not the first time we see these bugs, we should probably start nulling out rt->jitTop in debug builds after VM calls. And we could add a gczeal mode to trigger GC from the exception handler and other weird places...

Comment 3

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #2)
> It's not the first time we see these bugs, we should probably start nulling
> out rt->jitTop in debug builds after VM calls. And we could add a gczeal
> mode to trigger GC from the exception handler and other weird places...

This sounds like a great idea to me. Maybe this helps reproducing other intermittent bugs as well.

Comment 4

[Tooru Fujisawa [:arai]]

RegExpPrototypeOptimizableRaw was fallible from the beginning, but recently I added some more fallible code.
it may fail in LookupOwnPropertyPure called on RegExp.prototype, called from GetOwnGetterPure/GetOwnNativeGetterPure/HasOwnDataPropertyPure.
will check how it fail actually.

RegExpInstanceOptimizableRaw is not fallible at all.

Comment 5

[Tooru Fujisawa [:arai]]

maybe related to bug 1323108 that I fixed recently (regression in firefox52, fixed in firefox53)
before it gets fixed, RegExpPrototypeOptimizableRaw was calling GetGetterPure that may fail in more cases.

now I'm running jit-test with m-c 863c2b61bd27 + the patch, but I don't see crash so far.
will check with the revision before bug 1323108 patch later.

if bug 1323108 was the issue, it should be better uplifting it to firefox52.

Comment 6

[Tooru Fujisawa [:arai]]

after running jit-test some more, I hit the crash with m-c 863c2b61bd27 + the patch.
so looks like the issue persists after bug 1323108.

Comment 7

[Tooru Fujisawa [:arai]]

sorry, I misunderstood the patch.
with the patch the return value of RegExpPrototypeOptimizableRaw had no meaning :P

Comment 8

[Tooru Fujisawa [:arai]]

I'll change RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.
any failure in LookupOwnPropertyPure can be ignored if we only interested into successful case.
we can just treat all failure case non-optimizable.

anyway, bug 1323108 needs uplift, to uplift this.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Here's the patch.
Will ask review after running tests locally.

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8820510 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Removed `result` out-parameter from RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw, and now they return "optimizabale or not" as bool.

the behavior difference is that now we treat the failure of GetOwnGetterPure/GetOwnNativeGetterPure/HasOwnDataPropertyPure as non-optimizable
instead of failure.

in JIT code, they now stores the call result to output register directly.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8820510 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

Thanks for fixing this! I thought this affected ESR45 too, but I just checked and it landed after that, so that's great.

::: js/src/builtin/RegExp.cpp
@@ +1588,2 @@
>  {
>      JS::AutoCheckCannotGC nogc;

Nit: can also add AutoAssertNoPendingException aanpe(cx);

@@ +1680,2 @@
>  {
>      JS::AutoCheckCannotGC nogc;

And here.

::: js/src/jit/CodeGenerator.cpp
@@ -2121,5 @@
>  {
>      LRegExpPrototypeOptimizable* ins = ool->ins();
>      Register object = ToRegister(ins->object());
>      Register output = ToRegister(ins->output());
> -    Register temp = ToRegister(ins->temp());

We can now also remove the temp from LRegExpPrototypeOptimizable and LRegExpInstanceOptimizable.

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> ::: js/src/jit/CodeGenerator.cpp
> @@ -2121,5 @@
> >  {
> >      LRegExpPrototypeOptimizable* ins = ool->ins();
> >      Register object = ToRegister(ins->object());
> >      Register output = ToRegister(ins->output());
> > -    Register temp = ToRegister(ins->temp());
> 
> We can now also remove the temp from LRegExpPrototypeOptimizable and
> LRegExpInstanceOptimizable.

temp is still used in non-OOL path.
https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/js/src/jit/CodeGenerator.cpp#2101
https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/js/src/jit/CodeGenerator.cpp#2170

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Added AutoAssertNoPendingException.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

Sorry, you're right.

Comment 15

[Jan de Mooij [:jandem]]

In case you're not familiar with security bugs: don't forget to request sec-approval before landing this :)

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Thank you for reviewing :)


[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I don't know how to hit this without the extra patch in comment #0.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Not so much.
At least it doesn't describe how it can be hit.

> Which older supported branches are affected by this flaw?

mozilla-release (firefox 50)

> If not all supported branches, which bug introduced the flaw?

bug 887016 (from firefox 48)

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

not yet, but it would be easy.

> How likely is this patch to cause regressions; how much testing does it need?

less likely and automated test should be able to catch regression.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

sec-approval+ for trunk.
We'll want beta and aurora patches made and nominated once it lands on trunk and is good there.

Comment 18

[Tooru Fujisawa [:arai]]

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb892988ded8185e680a1328b0ff6c9b2bf8520

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3861ac69c78994cf478e07ce467880f881d7a45
Bug 1324810 - Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

Comment 20

[Tooru Fujisawa [:arai]]

backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3ea09dd511

https://treeherder.mozilla.org/logviewer.html#?job_id=40899737&repo=mozilla-inbound#L35341

hmm, not sure why it didn't hit on try run...

Comment 21

[Tooru Fujisawa [:arai]]

triggered some more run, and seems like a kind of conflict, not intermittent
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c3861ac69c78994cf478e07ce467880f881d7a45
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb892988ded8185e680a1328b0ff6c9b2bf8520

Comment 22

[Tooru Fujisawa [:arai]]

the issue on linux seems to be conflict with bug 1029245, that changes the gcc version.
now bisecting the issue on windows

Comment 23

[Tooru Fujisawa [:arai]]

confirmed the issue locally on linux64, and looks like we need "and 0xff" for return value, like IsConstructor
the return register contains random higher bits.

Comment 24

[Tooru Fujisawa [:arai]]

do you think we should always mask lower 8bits when getting bool return value of callWithABI to other register?

Comment 25

[Tooru Fujisawa [:arai]]

in that case maybe we should add masm.storeCallResultBool or something that does |masm.and32(Imm32(0xFF), output)| too.

Comment 26

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #24)
> do you think we should always mask lower 8bits when getting bool return
> value of callWithABI to other register?

Yeah that makes sense. We could add storeBoolCallResult (and maybe rename the other one to storeNonBoolCallResult?) that does this?

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}.

Renamed storeCallResult to storeCallWordResult, and added storeCallBoolResult that does "and 0xff".
and changed storeCallResult callsites to use either, depending on the return type or actual value range in "int" type.

storeCallBoolResult is defined in MacroAssembler-inl.h to call `and32`.

I'll update previous patch to use storeCallBoolResult and rename it to Part 1.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

how do we handle sec-approval for the case that the patch is updated later?

Comment 29

[Tooru Fujisawa [:arai]]

apparently windows hit the issue at the point of try run :P

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8821076 [details] [diff] [review]
Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}.

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

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ +446,5 @@
>  
>          masm.setupUnalignedABICall(temp0);
>          masm.passABIArg(temp1);
>          masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, GrowBacktrackStack));
> +        masm.storeCallBoolResult(temp0);

This returns an int so should use storeCallWordResult

@@ +865,5 @@
>          } else {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareUCStrings;
>              masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, fun));
>          }
> +        masm.storeCallBoolResult(temp0);

And here.

::: js/src/jit/MacroAssembler.cpp
@@ +1978,5 @@
>      if (compilingWasm)
>          callWithABI(wasm::SymbolicAddress::ToInt32);
>      else
>          callWithABI(mozilla::BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +    storeCallBoolResult(dest);

This is calling the JS::ToInt32 version that returns int32_t I think, so we should use storeCallWordResult.

::: js/src/jit/SharedIC.cpp
@@ +1355,5 @@
>          masm.push(intReg);
>          masm.setupUnalignedABICall(scratchReg);
>          masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
>          masm.callWithABI(mozilla::BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(scratchReg);

And here.

@@ +1508,5 @@
>          masm.bind(&truncateABICall);
>          masm.setupUnalignedABICall(scratchReg);
>          masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
>          masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(scratchReg);

And here.

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +865,5 @@
>          if (gen->compilingWasm())
>              masm.callWithABI(wasm::SymbolicAddress::ToInt32);
>          else
>              masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(output);

And here.

@@ +950,5 @@
>              masm.callWithABI(wasm::SymbolicAddress::ToInt32);
>          else
>              masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
>  
> +        masm.storeCallBoolResult(output);

And here.

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 8821078 [details] [diff] [review]
Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

It's fine to land these patches, the changes are pretty minor.

Comment 32

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c11875a3653b74939b44980d50bb6c05386d46
Bug 1324810 - Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f897d39a026697efbe0f8a8f4c734dbbefde7e
Bug 1324810 - Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31c11875a365
https://hg.mozilla.org/mozilla-central/rev/12f897d39a02

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Approval Request Comment
> [Feature/Bug causing the regression]

bug 887016

> [User impact if declined]

possible crash

> [Is this code covered by automated tests?]

no
we don't have any reproducible test for the crash.

> [Has the fix been verified in Nightly?]

partly
at least it's working, but there's no dedicated test that verifies the issue

> [Needs manual test from QE? If yes, steps to reproduce]

no

> [List of other uplifts needed for the feature/fix]

none

> [Is the change risky?]

less risky

> [Why is the change risky/not risky?]

It changes only the failure path, to take less risky path than previously taken dangerous path.

> [String changes made/needed]

none

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Approval Request Comment
> [Feature/Bug causing the regression]

bug 887016

> [User impact if declined]

possible crash

> [Is this code covered by automated tests?]

no
we don't have any reproducible test for the crash.

> [Has the fix been verified in Nightly?]

partly
at least it's working, but there's no dedicated test that verifies the issue

> [Needs manual test from QE? If yes, steps to reproduce]

no

> [List of other uplifts needed for the feature/fix]

none

> [Is the change risky?]

less risky

> [Why is the change risky/not risky?]

It changes only the failure path, to take less risky path than previously taken dangerous path.

> [String changes made/needed]

none

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

Comment 38

[Tooru Fujisawa [:arai]]

patches for mozilla-aurora has no change from mozilla-central.

patches for mozilla-beta has some change:
  Part 0: only surrounding code change
  Part 1: RegExpPrototypeOptimizableRaw checks less properties

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821442 [details] [diff] [review]
(mozilla-aurora) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Sec-critical fix, should prevent crashes, OK to land on aurora.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821443 [details] [diff] [review]
(mozilla-aurora) Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

2nd part for aurora fix. 
Thanks arai for making it super clear what should land on which branch!

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821444 [details] [diff] [review]
(mozilla-beta) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Let's aim this at the 51 beta 11 build.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe80ac73a112
https://hg.mozilla.org/releases/mozilla-aurora/rev/1458ff272561

https://hg.mozilla.org/releases/mozilla-beta/rev/6f93876ab2c8
https://hg.mozilla.org/releases/mozilla-beta/rev/addb771c9a72