Closed Bug 949475 Opened 6 years ago Closed 6 years ago

Add extra debug checks for MIR return values

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: sec-other, Whiteboard: [adv-main29-])

Attachments

(2 files, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Bug 942530 is a Windows-only GC assert that's pretty complicated to track down.

This patch adds type checks after all MIR instructions that return a Value/string/object. These check the values are sane GC pointers and the type is in the instruction's resultTypeSet (if it has one).

When I run the problematic website, these checks fail: it looks like we have a string pointer that's pointing to a JSObject. The AllocKind assert in AssertValidStringPtr catches this kind of thing.

I'd like to get some fuzzing of this patch before I land it. It passes jit-tests with --ion-eager, except for one test that does something like this:

function f() {
    for (var j = 0; j < 5; j++) {}

    function foo() {}
    foo.__proto__ = [];

    for (var j = 0; j < 20; j++)
	foo.length;
}
enableIonTypeChecks();
f();

This fails because |foo.__proto__ = []| changes foo's TypeObject, without invalidating the script.

We do give the type "unknown properties" in SetClassAndProto, so I'm not sure if it's a serious problem, but it still seems scary because we may look at the TypeObject's proto etc. Brian, what do you think?

If it's not a real issue, any idea how I can work around this? I could just disable the type check if there's a TypeObject with unknown properties in the TypeSet, but that seems a bit unfortunate.
Attachment #8346568 - Flags: review?(bhackett1024)
Flags: needinfo?(bhackett1024)
Even if IonBuilder looks at the type object's proto, it shouldn't be able to do any useful optimizations because the properties of the object itself are unknown.

Still, the basic TI invariant is that type sets capture the exact types of the values they can contain.  The type set walk we do when mutating prototypes is designed to preserve this invariant.  If we leave this IonBuilder behavior the way it is then this invariant can get violated --- suppose f() wrote foo to some heap structure without accessing any of f's properties.

So a few options I see:

- Degrade the TI invariant so that type sets containing objects with unknown properties could in fact contain other objects.  Per the above I can't think offhand about any bad behavior this would really cause, but it could have wonky effects on barriers and would make things more complex for anyone wanting to implement new optimizations in IonBuilder based on type set contents (e.g. class or proto based optimizations).  So for that reason I think we should keep the TI invariant the way it is and fix this testcase.  That can be done in one of two ways:

- Make JSOP_LAMBDA a JOF_TYPESET opcode and put the object's type in it.  Then the type set crawl on the proto set would find this type set, mark it unknown and invalidate the script.  We already do this for JSOP_NEWARRAY/NEWOBJECT/etc. but I don't know if that code is even active anymore --- it really dates back to JM days.  A better option on cleanliness and memory usage would be to remove the JOF_TYPESET from the initializer opcodes and use a new mechanism for Ion:

- Pass in a CompilerConstraintList to MakeSingletonTypeSet and add a type constraint which triggers invalidation of the script if the input type object acquires unknown properties, via TypeObjectKey::hasFlags.  This would handle lambdas, new arrays and objects and all these other places where we make type sets used in Ion without having an input StackTypeSet which will be caught by the type set crawl above.
Flags: needinfo?(bhackett1024)
Great to see invariants being hammered out. Can we have these checks on by default in DEBUG builds? They are logically like assertions, right?
(In reply to Brian Hackett (:bhackett) from comment #1)
> - Pass in a CompilerConstraintList to MakeSingletonTypeSet and add a type
> constraint which triggers invalidation of the script if the input type
> object acquires unknown properties, via TypeObjectKey::hasFlags.  This would
> handle lambdas, new arrays and objects and all these other places where we
> make type sets used in Ion without having an input StackTypeSet which will
> be caught by the type set crawl above.

Thanks, I will give this a try.

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Great to see invariants being hammered out. Can we have these checks on by
> default in DEBUG builds? They are logically like assertions, right?

I don't think it should be on by default: (1) These checks are separate MIR/LIR instructions, and influence register allocation etc. This may hide other problems or make it harder to reproduce problems with a debug build. (2) These checks are pretty slow because they add a ton of extra VM calls. Each of these calls involves a bunch of pushing/popping to save volatile registers.

We can add a shell flag though so that we can test this configuration with jit_test.py --tbpl, like the range analysis checks (--ion-check-range-analysis).
Fuzzing (or compilation, for that matter) on Windows is currently blocked by bug 948301.

Adding a shell flag works for us, btw.
Attachment #8346568 - Flags: review?(bhackett1024)
Attached patch Part 1 - Fix failing test (obsolete) — Splinter Review
This patch removes JOF_TYPESET from JSOP_NEWARRAY/NEWOBJECT/NEWINIT, and makes MakeSingletonTypeSet invalidate when the type gets unknown properties.

Now all jit-tests pass with the extra checks and --ion-eager. Benchmarks seem unaffected by this change.
Attachment #8346568 - Attachment is obsolete: true
Attachment #8347155 - Flags: review?(bhackett1024)
Forgot to qref.
Attachment #8347155 - Attachment is obsolete: true
Attachment #8347155 - Flags: review?(bhackett1024)
Attachment #8347157 - Flags: review?(bhackett1024)
Attached patch Part 2 - Add type checks (obsolete) — Splinter Review
Adds --ion-check-types flag instead of a function.
Attachment #8347165 - Flags: review?(bhackett1024)
Comment on attachment 8347157 [details] [diff] [review]
Part 1 - Fix for failing testcase

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

::: js/src/jit/IonBuilder.cpp
@@ +3590,1 @@
>      current->add(ins);

Can you use IonBuilder::constant() here?

::: js/src/jit/MIR.cpp
@@ +445,5 @@
>      setResultType(MIRTypeFromValue(vp));
>      if (vp.isObject()) {
>          // Create a singleton type set for the object. This isn't necessary for
>          // other types as the result type encodes all needed information.
> +        JS_ASSERT(constraints);

This assert might be better off in MakeSingletonTypeSet.
Attachment #8347157 - Flags: review?(bhackett1024) → review+
Comment on attachment 8347165 [details] [diff] [review]
Part 2 - Add type checks

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

The idea here is great, but it would be really nice if this could be done in a way that didn't influence regalloc or other optimizations.  Then we would be closer to making these assertions always-on in debug builds or on tbpl or whatever.  It seems like this could be done in CodeGenerator::generateBody, adding instrumentation as we do with the block counts stuff.  After suitable LIR instructions (associated with a MIR instruction where the type() of the MIR does not capture everything in resultTypeSet()), add some code to check the value produced by the LIR instruction matches the type set of the MIR, using push/pop to avoid requiring additional registers.

(While the range assertion checking stuff follows along with your patch here, I think that would be better done this way as well.)
Attachment #8347165 - Flags: review?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #9)
> The idea here is great, but it would be really nice if this could be done in
> a way that didn't influence regalloc or other optimizations.

Fair enough, I can try this. The main issue is that if the IonScript is invalidated we'll still run this code until we reach the OsiPoint. Values with an invalid type are expected in this case, but we can probably make that work somehow by checking if the IonScript was invalidated..

FWIW, bug 942530's website does not fail any of these checks with a 32-bit build on OS X, but on Windows it fails pretty soon. It looks lik we have a MLoadSlot for the global that loads a function with an unexpected TypeObject. That website is a disaster to debug; tens of thousands of lines of code translated from Java I think, and the global has 65535 dynamic slots :(
Attached patch Part 2 - Add type checks (obsolete) — Splinter Review
Now emits the checks as part of codegen in debug builds after the LIR instruction. You were right, this is a bit nicer.

I ran jit-tests without the first patch, and the test still fails so this patch works fine.

I want to push this to Try and see how much the VM calls affect test times. If it doesn't make the tests significantly slower, I'd like to leave them on in debug builds. Else I will make them optional and add a pref.
Attachment #8347165 - Attachment is obsolete: true
Attachment #8347390 - Flags: review?(bhackett1024)
Fix a last-minute thinko that broke some jit-tests.
Attachment #8347390 - Attachment is obsolete: true
Attachment #8347390 - Flags: review?(bhackett1024)
Attachment #8347486 - Flags: review?(bhackett1024)
Comment on attachment 8347486 [details] [diff] [review]
Part 2 - Add type checks

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

Looks great.

::: js/src/jit/CodeGenerator.cpp
@@ +2914,5 @@
> +
> +    if (mir->type() == MIRType_Object &&
> +        mir->resultTypeSet() &&
> +        !mir->resultTypeSet()->unknownObject() &&
> +        mir->resultTypeSet()->getObjectCount() > 0)

What is the getObjectCount() test for?  It seems like if getObjectCount() == 0 here then the type set guard should always miss rather than be skipped.
Attachment #8347486 - Flags: review?(bhackett1024) → review+
Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd9d75fe43b

I think it's ok to let this ride the trains; it's a pretty big patch and we haven't found any concrete problems. It would be good to keep this bug closed for a while though.
(In reply to Brian Hackett (:bhackett) from comment #13)
> What is the getObjectCount() test for?  It seems like if getObjectCount() ==
> 0 here then the type set guard should always miss rather than be skipped.

guardObjectType asserts getObjectCount() > 0. I changed the type check to always miss if getObjectCount() == 0.
https://hg.mozilla.org/mozilla-central/rev/1bd9d75fe43b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 986678
Whiteboard: [adv-main29-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.