1128196 - Skipping argument type checks is unsafe when the callee is relazified

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

I did some Try pushes with more eager JIT compilation, and it triggered a bunch of mysterious argument-check failures on various test suites. I think what's happening is this:

(1) We have a callee function (ArrayIndexOf in the case I looked at), that has a TypeScript. Let's call this function G.

(2) We Ion-compile a function (F) that calls G. We use G's TypeScript to determine we can skip the argument type checks for this call.

(3) We relazify G.

(4) G is unlazified, gets a new JSScript with a new TypeScript, new Baseline/Ion code.

(5) F's Ion code is still the same and it can call G and skip the argument checks. However, this is no longer sound, because G has a new TypeScript and its argument types are no longer guaranteed to match...

Comment 1

[Jan de Mooij [:jandem]]

Btw, it seems like this can happen whenever we discard a script's TypeScript without invalidating *all* Ion code in the compartment... Relazification can do this but I'm not sure it's the only way.

Comment 2

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 3

[Till Schneidereit [:till]]

Blergh. Either we change relazification to only be triggered when we also invalidate all Ion code, or we keep the TypeScript around, I guess. I kind of like the former, because it might also get rid of other subtle issues, and the memory usage regresssions shouldn't be too important as long as we still relazify on memory pressure.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Shell testcase

Repros at inbound revision 9758962dd013.

$ js --no-threads test.js
Assertion failure: Argument check fail., at MacroAssembler.cpp:1334

Very hard to generate for a fuzzer though, the test relies on a lot of factors and heuristics.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #3)
> Either we change relazification to only be triggered when we also
> invalidate all Ion code, or we keep the TypeScript around, I guess.

I don't know if the former is safe with incremental GC, unless we discard all Ion code on every slice.

Another option is to only relazify if the script has no TypeScript. TypeScripts are not discarded on every GC (see JIT_SCRIPT_RELEASE_TYPES_PERIOD), but after a number of GCs we should discard the types and then we can relazify...

Comment 6

[Jan de Mooij [:jandem]]

Also, it'd be nice if we could get rid of this footgun:

(In reply to Jan de Mooij [:jandem] from comment #1)
> it seems like this can happen whenever we discard a script's TypeScript
> without invalidating *all* Ion code in the compartment...

Maybe we should change our skip-argument-checks code to invalidate the caller whenever the callee's TypeScript goes away. Or something...

Comment 7

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Another option is to only relazify if the script has no TypeScript.
> TypeScripts are not discarded on every GC (see
> JIT_SCRIPT_RELEASE_TYPES_PERIOD), but after a number of GCs we should
> discard the types and then we can relazify...

That makes sense to me, yes.

(In reply to Jan de Mooij [:jandem] from comment #6)
> Maybe we should change our skip-argument-checks code to invalidate the
> caller whenever the callee's TypeScript goes away. Or something...

And this, too.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

Don't relazify scripts with a TypeScript.

Later we can add a comment and remove the hasBaselineScript and hasIonScript checks but this is just the simplest/safest fix for now.

Comment 9

[Till Schneidereit [:till]]

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

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

Delightful.

Comment 10

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, it'd require quite a lot of work.

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

> Which older supported branches are affected by this flaw?
All of them, I think.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy, patch is a one-liner.

> How likely is this patch to cause regressions; how much testing does it need?
It could regress memory usage as we will potentially relazify functions at a later point, but I don't think this will be a problem because we will still relazify them at some point.

Comment 11

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

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

sec-approval+.

Let's get this on Aurora, Beta, and ESR31 ASAP too.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1938c82eaf52

Comment 13

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/regressing bug #]: Bug 886193.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: Just landed on m-i; safe one-liner.
[Risks and why]: Could regress memory usage slightly, because we now have to wait with relazifying functions until the TypeScripts are discarded. However I'm not too worried about this.
[String/UUID change made/needed]: None.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Best to get this on all channels at once. Approving for ESR31, Beta, and Aurora.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1938c82eaf52

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/106557b12647
https://hg.mozilla.org/releases/mozilla-beta/rev/2f97ce37f5c4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/106557b12647
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/c2a0c1fade1a
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d69e71bb887f
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c202421662c7
https://hg.mozilla.org/releases/mozilla-esr31/rev/a95c84cc3316

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Less bustagetastic on <36.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4bb90badb575
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d47ec000ee85
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/db828c1ba599
https://hg.mozilla.org/releases/mozilla-esr31/rev/3ae8a649c97e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/4bb90badb575
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/d47ec000ee85