Closed Bug 1324810 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: jandem, Assigned: arai)

Details

(Keywords: sec-critical, Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(7 files, 2 obsolete files)

2.47 KB, patch
Details | Diff | Splinter Review
13.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
13.13 KB, patch
arai
: review+
Details | Diff | Splinter Review
10.27 KB, patch
arai
: review+
Details | Diff | Splinter Review
13.18 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.08 KB, patch
arai
: review+
Details | Diff | Splinter Review
Attached patch Trigger crashSplinter Review
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.
Keywords: sec-critical
Priority: -- → P1
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.
Flags: needinfo?(arai.unmht)
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...
(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.
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.
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.
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.
sorry, I misunderstood the patch.
with the patch the return value of RegExpPrototypeOptimizableRaw had no meaning :P
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.
Here's the patch.
Will ask review after running tests locally.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
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.
Attachment #8820510 - Flags: review?(jdemooij)
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.
Attachment #8820510 - Flags: review?(jdemooij)
(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
Added AutoAssertNoPendingException.
Attachment #8820510 - Attachment is obsolete: true
Attachment #8820728 - Flags: review?(jdemooij)
Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

Sorry, you're right.
Attachment #8820728 - Flags: review?(jdemooij) → review+
In case you're not familiar with security bugs: don't forget to request sec-approval before landing this :)
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.
Attachment #8820728 - Flags: sec-approval?
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.
Attachment #8820728 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3861ac69c78994cf478e07ce467880f881d7a45
Bug 1324810 - Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings
the issue on linux seems to be conflict with bug 1029245, that changes the gcc version.
now bisecting the issue on windows
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.
do you think we should always mask lower 8bits when getting bool return value of callWithABI to other register?
Flags: needinfo?(jdemooij)
in that case maybe we should add masm.storeCallResultBool or something that does |masm.and32(Imm32(0xFF), output)| too.
(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?
Flags: needinfo?(jdemooij)
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.
Attachment #8821076 - Flags: review?(jdemooij)
how do we handle sec-approval for the case that the patch is updated later?
Attachment #8820728 - Attachment is obsolete: true
Attachment #8821078 - Flags: review?(jdemooij)
apparently windows hit the issue at the point of try run :P
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.
Attachment #8821076 - Flags: review?(jdemooij) → review+
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.
Attachment #8821078 - Flags: review?(jdemooij) → review+
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
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
Attachment #8821442 - Flags: review+
Attachment #8821442 - Flags: approval-mozilla-aurora?
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
Attachment #8821444 - Flags: review+
Attachment #8821444 - Flags: approval-mozilla-beta?
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
Group: javascript-core-security → core-security-release
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.
Attachment #8821442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Attachment #8821443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8821444 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8821445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.