1437842 - Crash [@ ??] with GC and TypedArray constructors

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2b7d42d527af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions --ion-extra-checks --baseline-eager --ion-eager --ion-offthread-compile=off):

try { eval(`
try { evaluate(\`
(function(global) {
    const {
    } = WeakMap.prototype;
    function sharedConstructor(baseConstructor) {
        class SharedTypedArray extends Object.getPrototypeOf(baseConstructor) {}
        return SharedTypedArray;
    }
    const typedArrayConstructors = Object.freeze([
        Uint8ClampedArray,
    ]);
    const sharedTypedArrayConstructors = Object.freeze(
        typeof SharedArrayBuffer === "function"
        ? typedArrayConstructors.map(sharedConstructor)
        : []
    );
    const anyTypedArrayConstructors = Object.freeze([
        ...typedArrayConstructors, ...sharedTypedArrayConstructors,
    ]);
    global.anyTypedArrayConstructors = anyTypedArrayConstructors;
  global.assertThrowsInstanceOf = function assertThrowsInstanceOf(f, ctor, msg) {
    try {
    } catch (exc) {
    }
  };
  global.assertDeepEq = (function(){
    function isPrimitive(v) {
    };
  })();
})(this);
\`); } catch(exc) {}
evaluate(\`
    gczeal(14,18);
\`);
while (true) {
  try { evaluate(\`
   for (var constructor of anyTypedArrayConstructors) {
    var hits = 0, obj = {};
    function f(x) {}
    constructor.from(["a", "b", "c"], f, obj);
    function gs(x) {}
    constructor.from("def", gs);
   }
  \`); } catch(exc) {}
}
`); } catch (exc) {}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00001ce2ef1b2c2f in ?? ()
#0  0x00001ce2ef1b2c2f in ?? ()
[...]
#32 0x0000000000000000 in ?? ()
rax	0x7ffff5900250	140737313243728
rbx	0x57ffff59001f0	1548112196796912
rcx	0x7ffff58aa3d0	140737312891856
rdx	0x0	0
rsi	0xfffe7ffff5900290	-422212640177520
rdi	0xfff8800000000000	-2111062325329920
rbp	0x0	0
rsp	0x7fffffffac30	140737488333872
r8	0xfff9800000000000	-1829587348619264
r9	0x20	32
r10	0x2a000	172032
r11	0x1fff1	131057
r12	0x0	0
r13	0x7ffff5900290	140737313243792
r14	0x7ffff5900250	140737313243728
r15	0x7fffffffb460	140737488335968
rip	0x1ce2ef1b2c2f	31760999722031
=> 0x1ce2ef1b2c2f:	mov    0x4(%rbx),%esi
   0x1ce2ef1b2c32:	movabs $0x7ffff5f680e8,%r11



This test was initially highly intermittent. I made it much more reliable by adding the "while (true)" construct around the end of the original test and find a second gczeal parameter that would reproduce deterministically. It would be highly appreciated if we could figure out what makes this particular crash so hard to reproduce. Initially, I had a less reduced test with gczeal(14,9) and without the loop, and it would reproduce in 2% of all runs, even with --no-threads. According to :jonco, that rules out background sweeping. So there must be other reasons for non-determinism at play here. Also it would be good to know why/how the loop can stabilize this situation. Note that this does *not* reproduce on debug builds.

Marking s-s and sec-high because this is a scary JIT GC crash.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5d6cc408dfd9
user:        Matthew Gaudet
date:        Tue Dec 12 16:21:45 2017 -0600
summary:     Bug 1420910: Convert the Baseline InstanceOf IC to CacheIR r=jandem

This iteration took 228.039 seconds to run.

Comment 2

[Jon Coppeard (:jonco)]

I reproduced this under rr.  Using rsi I stepped back to the last place where the debugger can understand the stack, which was DoWarmUpCounterFallbackOSR.  This crash is occurring shortly after returning from this function.

I build an opt build with JIT spew enabled and turned on bl-osr.  I saw the following output:

[BaselineOSR]   OSR possible!
[BaselineOSR] Got jitcode.  Preparing for OSR into ion.
[BaselineOSR] Allocated IonOsrTempData at 7ffff54ce2e0
[BaselineOSR] Jitcode is 1c1456fb879a

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
js/src/gdb/mozilla/asmjs.py: Allowing WasmFaultHandler to run.

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
0x00001c1456fb8c61 in ?? ()
0x00001c1456fb8c61 in ?? ()

This indicates that the crash is occurring just after we start executing JIT code.

I don't think this is GC related per se.  However GC does discard JIT code code so I suspect that is why gczeal is need to reproduce this.

Comment 3

[Jan de Mooij [:jandem]]

I think the problem is the following code in StringIteratorNext:

    if (!IsObject(this) || !IsStringIterator(this)) {
        return ...;
    }

    var S = UnsafeGetStringFromReservedSlot(this, ITERATOR_SLOT_TARGET);
    ...
    var size = S.length;

What's happening is that we inline this function in some loop, then we loop-hoist the slot load + S.length. These instructions are moved *before* the IsStringIterator check and that's obviously unsafe.

This was buggy and maybe unsafe before, but because of the Spectre Value mitigations (bug 1432479), we now get a guaranteed crash because we unbox an object (we have an ArrayIteratorObject instead of StringIteratorObject) as string.

Comment 4

[Jeff Walden [:Waldo]]

(adding CCs for information and bug-avoidance purposes especially for people routinely writing self-hosted JS)

Comment 5

[Jan de Mooij [:jandem]]

An easy "fix" is to mark the slot load (UnsafeGetStringFromReservedSlot) as non-movable in Ion. The main question is whether there are other cases we need to worry about. For instance, other intrinsics that are inlined in Ion, are movable, and rely on a particular object class.

Comment 6

[Tom S [:evilpie]]

I guess the harder fix would be to change the AliasSet? I.e have IsStringIterator etc. have an alias set of  AliasSet::Store(AliasSet::FixedSlot)? This seem quite error prone, we probably have other kinds of instructions with the same problem, that use a different set. I think in reality we want something like AliasSet::Store(Type(StringIter)) on the IsStringIterator, and have LoadFixedSlot be AliasSet::Load(Type(StringIter)). This should ensure we don't hoist the load before the IsType check.

Comment 7

[André Bargull [:anba]]

Attachment: bug1437842-testcase-no-forof.js

I was actually planing on finally landing bug 1407588, but that will break the fuzz tests (from comment#0 and also from bug 1438137). So here's a reduced test case which still breaks for me most of the time, while keeping this issue reproducible with and without the patches from bug 1407588.

Comment 8

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 27fd083ed7ee).

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to André Bargull [:anba] from comment #7)
> Created attachment 8950919 [details]
> bug1437842-testcase-no-forof.js

Using this testcase (still intermittent though arguably reliable),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6e9b4b746ee6
user:        Jan de Mooij
date:        Wed Jan 24 12:33:53 2018 +0100
summary:     Bug 1432479 - Use XOR for Value unboxing on 64-bit to mitigate certain Spectre attacks. r=luke

Jan, perhaps bug 1432479 is the real regressor?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> Jan, perhaps bug 1432479 is the real regressor?

No, see comment 3:

> This was buggy and maybe unsafe before, but because of the Spectre Value mitigations (bug 1432479), we now get a guaranteed crash because we unbox an object (we have an ArrayIteratorObject instead of StringIteratorObject) as string.

Comment 11

[Jan de Mooij [:jandem]]

This bug is pretty bad and we need an owner to do an audit at least. I'm currently busy with Spectre stuff I want to land this release cycle. Ted, maybe you can take this?

Comment 12

[Ted Campbell [:tcampbell]]

I'm happy to take it Monday when I'm back. We'll be tight on time but it should be possible. An example patch for the known case would be appreciated, and then I can do a larger audit of other cases.

Comment 13

[Jan de Mooij [:jandem]]

Hm I just realized this is also a problem for Spectre object type mitigations.

When self-hosted code does something like this:

  if (!IsFooObject(o))
    return ...;
  UnsafeGetStringFromReservedSlot(o, x);

We can move the slot load before the class check (that's this bug) but to mitigate Spectre we also really want to have an explicit data dependency here. Then IsFooObject can for instance zero the object register on speculative paths to ensure the UnsafeGetStringFromReservedSlot is harmless.

So that suggests the following redesign, to kill two birds with one stone: we replace IsFooObject with GuardIsFooObject and make it return null if the object doesn't match, or the object if it does. Then the self-hosted code will look like this:

  o = GuardIsFooObject(o);
  if (o === null)
    return ...;
  UnsafeGetStringFromReservedSlot(o, x);

Unfortunately it's still possible to write unsafe code then:

  if (GuardIsFooObject(o) === null)
     return ...;
  UnsafeGetStringFromReservedSlot(o, x); // No dependency, still vulnerable.

Maybe we could call these intrinsics GuardToFooObject (To instead of Is) to make this pattern less likely.

Thoughts?

Comment 14

[Luke Wagner [:luke]]

Are there reasons the Unsafe* operations can't be merged with the Is* operations to make a safe operation?  (That seems safer in general, so I'm guessing there's a reason this can't work, but I thought I'd ask.)

Comment 15

[Frederik Braun [:freddy]]

This bug is unassigned, inactive yet it has a test case and a high security rating. Can you help us find an owner, Luke?

Comment 16

[Luke Wagner [:luke]]

Jan seems to have a handle on the problem.

Comment 17

[Jan de Mooij [:jandem]]

sdetar, this is the bug I mentioned earlier this week.

Comment 18

[Steven DeTar [:sdetar]]

mgaudet is going to look into to the bug and see if he can take it.

Comment 19

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: WIP For converting checks to use "Guard-return" style

Comment 20

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8965456 [details] [diff] [review]
WIP For converting checks to use "Guard-return" style

Just want to run this by you Jan, to see what you think. 

Now, I'm not sure if this changes the determination of blame, but I can say: This patch is insufficient to stop failures on the test case in comment 7. 

I have got this in rr though, so can do more determination later.

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8965456 [details] [diff] [review]
WIP For converting checks to use "Guard-return" style

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

So this works, but I'm not sure if we need this for ToObject. IIRC the problem here is IsStringIterator (and other Is* intrinsics) and changing that one *should* fix the crash :)

::: js/src/builtin/String.js
@@ +542,5 @@
>  }
>  
>  function StringIteratorNext() {
> +    var obj = GuardToObject(this);
> +    if (obj == null || !IsStringIterator(this)) {

Nit: use === instead of ==

Comment 22

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Preliminary Fix for StringIteratorNext

Comment 23

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8965785 [details] [diff] [review]
Preliminary Fix for StringIteratorNext

This patch is a basic fix for the test case in comment 7 (in my testing, the case stops reproducing, and all the jit tests pass locally w/ 64bit). 

Note, this patch doesn't remove IsStringIterator, I'd do that as a separate patch. 

Not sure how to proceed after this as far as addressing the other cases: Is it best to start by auditing from the Unsafe{Get,Set}*Slot functions, and work backwards? 

Or should we blanket change all the functions to guards? 

Once this patch is reviewed, would we land this, or wait till we have the rest of the audit completed?

Looking for other instances of the |if (!IsObject() || !IsOtherType())| pattern w/ |IsObject.*\|\|.*!Is| I see the following as potentially functions of concern:

* IsArrayIterator
* IsGeneratorObject
* IsCollator
* IsDateTimeFormat
* IsNumberFormat
* IsPluralRules
* IsRelativeTimeFormat
* IsMapObject
* IsMapIterator
* IsModule
* IsRegExpObject
* IsSetObject
* IsSetIterator
* IsTypedArray
* IsSharedArrayBuffer

This list is of course preliminary, because of the limitations on its generation.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8965785 [details] [diff] [review]
Preliminary Fix for StringIteratorNext

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

Sorry for the delay. This looks really good! Hopefully with the framework in place fixing the remaining ones will be straight-forward. 

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #23)
> Not sure how to proceed after this as far as addressing the other cases: Is
> it best to start by auditing from the Unsafe{Get,Set}*Slot functions, and
> work backwards? 
> 
> Or should we blanket change all the functions to guards? 

I think auditing that way is a good idea, but we probably want to change all similar intrinsics (even the ones that are not vulnerable right now) just to avoid similar problems in the future (and it lets us use the same code!).

> Once this patch is reviewed, would we land this, or wait till we have the
> rest of the audit completed?

We should land them together (could be multiple patches).

::: js/src/jit/CodeGenerator.cpp
@@ +12642,5 @@
> +CodeGenerator::visitGuardToClass(LGuardToClass* ins)
> +{
> +    Register lhs = ToRegister(ins->lhs());
> +    Register output = ToRegister(ins->output());
> +    masm.breakpoint();

Nit: remove

@@ +12643,5 @@
> +{
> +    Register lhs = ToRegister(ins->lhs());
> +    Register output = ToRegister(ins->output());
> +    masm.breakpoint();
> +    masm.loadObjClassUnsafe(lhs, output);

We should use branchTestObjClass, it's simpler and we get Spectre mitigations

::: js/src/jit/MIR.h
@@ +13672,5 @@
> +      , class_(clasp)
> +    {
> +        MOZ_ASSERT(object->type() == MIRType::Object ||
> +                   (object->type() == MIRType::Value && object->mightBeType(MIRType::Object)));
> +        setResultType(MIRType::ObjectOrNull);

What worries me about this is that ObjectOrNull will prevent some optimizations, for instance UnsafeGetReservedSlot is only inlined if the object is an Object or Value [1].

I think it makes sense to do the following if we have |getInlineReturnType() == MIRType::Object|:

(1) Use MIRType::Object here instead of ObjectOrNull (maybe just pass the MIRType to this constructor and assert its Object || ObjectOrNull).
(2) In CodeGenerator, bail out instead of returning null if |mir->type() == MIRType::Object|.
(3) Here call setGuard() because we don't want to eliminate this instruction in that case.

This way we make sure the common always-object case is as fast as possible.

At some point we should probably also search for MIRType::Object checks we can turn into Object || ObjectOrNull, that would fix [1], but not in this bug :)

I'd also suggest testing some micro-benchmarks that hit this code to make sure there are no surprising perf regressions.

[1] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/js/src/jit/MCallOptimize.cpp#2999-3000

Comment 25

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Preliminary Fix for StringIteratorNext

Updated to address feedback from above. This also fixes a bug that caused
inlining to never occur. 

This continues to pass the test case, and passes all the JIT tests locally,
however investigation with a breakpoint shows that essentially no test case
in the test suite actually exercises this path. It appears throughout the 
suite that Ion already knows the type, and needn't emit MGuardToClass.

The only test case I have that does cause emission of the guard code is
the attached one. I'd love some help in writing one that causes the us to 
emit the MGuardTo that I understand :D 

A trivial string iterator micro-benchmark shows now slowdown, but, it's also
not hitting the GuardTo code, so.... not sure what that shows.

Comment 26

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsStringIterator to GuardToStringIterator

Comment 27

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Remove IsStringIterator intrinsic

Comment 28

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsArrayIterator to GuardToArrayIterator

Comment 29

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsMapIterator to GuardToMapIterator

Comment 30

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsSetIterator to GuardToSetIterator

Comment 31

[Matthew Gaudet (he/him) [:mgaudet]]

Audit Notes

I assumed if there was no attempt to guard, the guards existed elsewhere, and were sufficient. 

UnsafeGet*

* The above iterator patches seem to cover almost all the relevant cases (*cheers*)
* Not sure about Regex. Some questions there (e.g RegExpBuiltinExec, here: https://searchfox.org/mozilla-central/source/js/src/builtin/RegExp.js#991,1000) 
* TypedArray has an assert that seems questionable: https://searchfox.org/mozilla-central/source/js/src/builtin/TypedArray.js#20,32 but I can't decide if we care. 
* I couldn't make heads or tails of TypedObjects.js (Macros + complexity) 

UnsafeSet*

* Everything seems covered by the iterator patches.

Comment 32

[Jan de Mooij [:jandem]]

Comment on attachment 8967479 [details] [diff] [review]
Convert IsArrayIterator to GuardToArrayIterator

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

::: js/src/builtin/Array.js
@@ +708,5 @@
>  // http://www.ecma-international.org/ecma-262/6.0/index.html#sec-%arrayiteratorprototype%.next
>  function ArrayIteratorNext() {
>      // Step 1-3.
> +    var obj;
> +    if (!IsObject(this) || (obj = GuardToArrayIterator(this)) == null) {

Nit: === instead of ==

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8967480 [details] [diff] [review]
Convert IsMapIterator to GuardToMapIterator

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

::: js/src/builtin/Map.js
@@ +75,4 @@
>      var O = this;
>  
>      // Steps 2-3.
> +    if (!IsObject(O) || (O = GuardToMapIterator(O)) == null)

Nit: ===

Comment 34

[Jan de Mooij [:jandem]]

Comment on attachment 8967481 [details] [diff] [review]
Convert IsSetIterator to GuardToSetIterator

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

::: js/src/builtin/Set.js
@@ +73,4 @@
>      var O = this;
>  
>      // Steps 2-3.
> +    if (!IsObject(O) || (O = GuardToSetIterator(O)) == null)

Nit: ===

Comment 35

[Jan de Mooij [:jandem]]

Comment on attachment 8967477 [details] [diff] [review]
Convert IsStringIterator to GuardToStringIterator

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

Great! r=me with comments addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +12647,5 @@
> +
> +    Label done, notEqual;
> +
> +    masm.branchTestObjClass(Assembler::NotEqual, lhs, ins->mir()->getClass(), temp,
> +                          output, &notEqual);

Nit: indentation here looks wrong, add 2 more spaces?

@@ +12649,5 @@
> +
> +    masm.branchTestObjClass(Assembler::NotEqual, lhs, ins->mir()->getClass(), temp,
> +                          output, &notEqual);
> +    masm.mov(lhs, output);
> +    masm.jump(&done);

bailoutFrom does not emit any code, so we can move the |done| Label and this masm.jump/masm.bind into the else-branch below.

::: js/src/jit/MCallOptimize.cpp
@@ +2556,5 @@
> +
> +    if (callInfo.getArg(0)->type() != MIRType::Object)
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() != MIRType::ObjectOrNull &&
> +        getInlineReturnType() != MIRType::Object)

Nit: in this case (condition spanning multiple lines) we use {} to make it more clear where the condition ends and the body starts.

if (x && 
    y)
{

}

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Ready to land?  Does this need uplift? (likely)  This very likely also needs security-approval to land since it's almost certainly not Nightly-only.

Comment 37

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsStringIterator to GuardToStringIterator

Comment 38

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Remove IsStringIterator intrinsic

Comment 39

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsArrayIterator to GuardToArrayIterator

Comment 40

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsMapIterator to GuardToMapIterator

Comment 41

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert IsSetIterator to GuardToSetIterator

Comment 42

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add test case

Comment 43

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8969397 [details] [diff] [review]
Add test case

I'm a little conflicted about if we want to land this test case, just because it doesn't reproduce without a debug build, and takes almost two minutes to finish.

Comment 44

[Matthew Gaudet (he/him) [:mgaudet]]

Jan: Is there anything else you want looked at for this bug before we do the sec-approval and try to get this landed Monday (ie, you think the audit was sufficient, or you'd like it double checked etc?)

I'm not sure how far back you think this goes; certainly the crash origination is the Spectre mitigations in bug 1432479 (so, 60), but do you think it was potentially an exploitable flaw earlier?

Comment 45

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8969397 [details] [diff] [review]
Add test case

(Obsoleting test case patch to avoid landing it)

Comment 46

[Matthew Gaudet (he/him) [:mgaudet]]

(Also, these patches haven't gone through try...)

Comment 47

[Jan de Mooij [:jandem]]

I think it would be nice to at least remove IonBuilder::inlineHasClass and fix all the callers of that... Each of them potentially has the same issues, and this way we ensure we don't accidentally introduce a new buggy use. It should be just more of the same, but some of these might be perf sensitive. (We should probably also fix IsRegExpObject to be inlined the same way as its IsFoo siblings...)

We should uplift to beta/ESR 60 and ESR52 after it has been on Nightly for a few days.

Comment 48

[Matthew Gaudet (he/him) [:mgaudet]]

OK. I will take a look at that Monday. I can probably get through them, then run some local benchmarking.

Comment 49

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #47)
> (We should probably also fix IsRegExpObject to be inlined the same way as
> its IsFoo siblings...)

IsRegExpObject is slightly different because we wanted it to support MIRType::Value (bug 1395927).

Also: I guess this bug answered the reordering question in bug 1395927 comment #9. :-)

Comment 50

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 0] Convert IsStringIterator to GuardToStringIterator

Lays out the ground work

Comment 51

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 1] Remove IsStringIterator intrinsic

Comment 52

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2] Convert IsArrayIterator to GuardToArrayIterator

Comment 53

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 3] Convert IsMapIterator to GuardToMapIterator

Comment 54

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 4] Convert IsSetIterator to GuardToSetIterator

Comment 55

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 5] Convert IsCollator to GuardToCollator

Comment 56

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 6] Convert IsNumberFormat to GuardToNumberFormat

Comment 57

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 8] Convert IsDateTimeFormat to GuardToDateTimeFormat

Comment 58

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 9] Convert IsPluralRules to GuardToPluralRules

Comment 59

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 10] Convert IsRelativeTimeFormat to GuardToRelativeTimeFormat

Comment 60

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 11] Convert IsMapObject to GuardToMapObject

Comment 61

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970313 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

Carrying :jandem's r+ on Parts 0-4

Comment 62

[Matthew Gaudet (he/him) [:mgaudet]]

The currently attached series of twelve patches covers all the users of the inlineHasClass function, except for the TypedObject consumers [1]

Those consumers are used in a number of ways that don't seem to naturally map to the GuardTo structure the others use. [2]

(There's also a concern about how to implement inlineGuardToClass with multiple class pointers; HasClass can take advantage of bitwise operations to avoid control flow, but I am not sure there's an equivalent transform for GuardToClass) [3].  

What do we think about leaving the TypedObject code as is for now? 

[1]: https://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#375-396
[2]: https://searchfox.org/mozilla-central/source/js/src/builtin/TypedObject.js#1021
[3]: https://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#2546-2555

Comment 63

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #62)
> What do we think about leaving the TypedObject code as is for now? 

That's probably fine. TypedObjects are Nightly only (and will likely change a lot soon for wasm). We could file a follow-up bug to fix this separately.

Comment 64

[Jan de Mooij [:jandem]]

Comment on attachment 8970321 [details] [diff] [review]
[Part 9] Convert IsPluralRules to GuardToPluralRules

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

::: js/src/builtin/intl/CommonFunctions.js
@@ +1475,2 @@
>             (type === "RelativeTimeFormat" && IsRelativeTimeFormat(obj)),
> +           (type === "PluralRules" && GuardToPluralRules(obj) !== null) ||

This line should stay where it was. Also twice below.

Comment 65

[Jan de Mooij [:jandem]]

There's also IsSetObject and Is(Shared)ArrayBuffer to convert right?

Comment 66

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 10] Convert IsRelativeTimeFormat to GuardToRelativeTimeFormat

Comment 67

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 13] Convert IsArrayBuffer to GuardToArrayBuffer

Comment 68

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 14] Convert IsSharedArrayBuffer to GuardToSharedArrayBuffer

Comment 69

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970646 [details] [diff] [review]
[Part 10] Convert IsRelativeTimeFormat to GuardToRelativeTimeFormat

Carrying Jan's earlier r+, this just corrects a missed declaration that works fine under opt, but failed under a debug build.

Comment 70

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970648 [details] [diff] [review]
[Part 14] Convert IsSharedArrayBuffer to GuardToSharedArrayBuffer

Not sure how I missed those two, but yes, absolutely they needed to be covered. 

One comment about both the IsArrayBuffer and IsSharedArrayBuffer changes is that there's some sections that require the introduction of a new variable and name -- open to renaming

Comment 71

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970313 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

[Security approval request comment] (Request for Patches 0-14) 

How easily could an exploit be constructed based on the patch?

  It would be difficult to construct an exploit based on the patch. 

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

  The comments do not point to the issue directly. 

Which older supported branches are affected by this flaw?

  We believe this could go back to 52.

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

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

 I do not have backports as of yet, but believe based on inspection of the ESR52 code that they should be easy to do, and low risk. 

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

  These changes have run through Try [1], and cause a small (low-confidence) performance regression on Dromaeo-CSS and Kraken according to perfherder [2]. 

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd6d85e7091e4b042104ebe3e80525afcbe7418
[2]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=fdd6d85e7091&framework=1&showOnlyComparable=1&selectedTimeRange=172800

Comment 72

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

This is probably too late for 60. We're making release candidates after the weekend. I can talk to Release Management to consider it but... for sec-high and sec-critical security bugs (at the very least) you can't include a test in the security patch. That will zero day our users as soon as you check it in. 

I need a new patch nominated without the tests. You can attach a patch for just the tests but you won't be able to check it in until 3-4 weeks after we release a full release with the security fix in it. Set in-testsuite? to remind yourself to check the test in later.

Comment 73

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 0] Convert IsStringIterator to GuardToStringIterator

Lays out the ground work

Comment 74

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 5] Convert IsCollator to GuardToCollator

Comment 75

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 6] Convert IsNumberFormat to GuardToNumberFormat

Comment 76

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970968 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

Carrying r+, removed test code.

Comment 77

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970969 [details] [diff] [review]
[Part 5] Convert IsCollator to GuardToCollator

Carrying r+, edited patch to correct an ESLint error.

Comment 78

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970970 [details] [diff] [review]
[Part 6] Convert IsNumberFormat to GuardToNumberFormat

Carrying r+, edited patch to fix ESlint error.

Comment 79

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8970968 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

[Security approval request comment]
<see justification in Comment #71> 

Thanks for pointing that out. The test case that was included was -not- the vulnerable test case, just one I had written for ensuring the correctness of my own approach in changing things. 

I have uploaded a new patch that removes even that test, (and some ESLint fixes on two of the other patches, that were caught by the try build)

Comment 80

[Daniel Veditz [:dveditz]]

What happened to parts 7 and 12?

Is the sec-approval request for the whole bundle or just part 0 standalone?

Comment 81

[Matthew Gaudet (he/him) [:mgaudet]]

Part 7... is a counting failure on my part in my local patch stack.

Part 12 though: Thank you for noticing that I had missed it in my exports to bugzilla! 

In theory, this is for the whole stack: Is it better if I just replace all these with a rollup patch?

Comment 82

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 12] Convert IsSetObject to GuardToSetObject

Comment 83

[Daniel Veditz [:dveditz]]

What about the sec-approval though -- is that a standalone patch? Or are you seeking approval for everything?

Comment 84

[Matthew Gaudet (he/him) [:mgaudet]]

Seeking approval for the whole stack (first sec-high bug, sorry for this careening off the rails)

Comment 85

[Daniel Veditz [:dveditz]]

Comment on attachment 8970968 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

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

I'm super-dubious about attempting to uplift this into Firefox 60, but we can land on nightly now so it'll make Firefox 61 and then uplift to the ESR-60 branch in that cycle if this is stable.

sec-approval=dveditz

Comment 86

[Jan de Mooij [:jandem]]

Comment on attachment 8970981 [details] [diff] [review]
[Part 12] Convert IsSetObject to GuardToSetObject

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

Thanks!

Comment 87

[Matthew Gaudet (he/him) [:mgaudet]]

Pushed changesets: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?startID=102657&endID=102658 - 14 changesets

Comment 88

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

https://hg.mozilla.org/mozilla-central/rev/3574187cbf66
https://hg.mozilla.org/mozilla-central/rev/427ef5c6a478
https://hg.mozilla.org/mozilla-central/rev/be519ddd7453
https://hg.mozilla.org/mozilla-central/rev/e15b02478f78
https://hg.mozilla.org/mozilla-central/rev/c568e772c710
https://hg.mozilla.org/mozilla-central/rev/0993cb0d74e6
https://hg.mozilla.org/mozilla-central/rev/83ea97f7f8d3
https://hg.mozilla.org/mozilla-central/rev/bcbac9c78599
https://hg.mozilla.org/mozilla-central/rev/8206293b20c0
https://hg.mozilla.org/mozilla-central/rev/314bb18aab52
https://hg.mozilla.org/mozilla-central/rev/7b62cd702024
https://hg.mozilla.org/mozilla-central/rev/c49e30fa8764
https://hg.mozilla.org/mozilla-central/rev/4e618d287d81
https://hg.mozilla.org/mozilla-central/rev/b96020a79cfa

Comment 89

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 90

[Ryan VanderMeulen [:RyanVM]]

Based on Dan's comments, it sounds like we'll need rebased patches for both ESR60 and ESR52.

Comment 91

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change inlining of intrinsics (ESR52)

Rollup patch for backport.

Comment 92

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change inlining of intrinsics (ESR60)

Rollup patch for backport

Comment 93

[Matthew Gaudet (he/him) [:mgaudet]]

Backported patches attached. They've not been tested via try (can we do try builds of older releases?), but do build and run locally.

Comment 94

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8972162 [details] [diff] [review]
Change inlining of intrinsics (ESR52)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

  It is possible for a security vulnerability to be developed around the Ion hoisting failure. 

Fix Landed on Version: Nightly 

Risk to taking this patch (and alternatives if risky): The original version of this patch has been on nightly for a few days with no reported regressions. There's a chance of performance regression (though, hasn't been born out in nightly). 

String or UUID changes made by this patch: None.

Comment 95

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8972163 [details] [diff] [review]
Change inlining of intrinsics (ESR60)

(Carrying the review from the original 

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

  It is possible for a security vulnerability to be developed around the Ion hoisting failure. 

Fix Landed on Version: Nightly 

Risk to taking this patch (and alternatives if risky): The original version of this patch has been on nightly for a few days with no reported regressions. There's a chance of performance regression (though, hasn't been born out in nightly). 

String or UUID changes made by this patch: None.

Comment 96

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We can aim this fix at 52.9.0 but it will miss 52.8.0. For now, I'd like to leave this in the approval queue (until we actually release 52.8.0 and 60.0)

Comment 97

[Julien Cristau [:jcristau]]

Comment on attachment 8972162 [details] [diff] [review]
Change inlining of intrinsics (ESR52)

sec-high fix for 52.9esr

Comment 98

[Julien Cristau [:jcristau]]

Comment on attachment 8972163 [details] [diff] [review]
Change inlining of intrinsics (ESR60)

.. and for 60.1esr

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a475573d2923

Comment 100

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/eb896089db473fca38a72cbf12ea40906989a602