Closed Bug 1437842 Opened 3 years ago Closed 3 years ago

Crash [@ ??] with GC and TypedArray constructors

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 61+ fixed
firefox-esr60 61+ fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: decoder, Assigned: mgaudet)

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main61+][adv-esr52.9+][adv-esr60.1+] in-testsuite? )

Crash Data

Attachments

(17 files, 18 obsolete files)

687 bytes, application/x-javascript
Details
2.11 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
4.18 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
2.84 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
3.35 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
6.92 KB, patch
jandem
: review+
Details | Diff | Splinter Review
6.66 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.69 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.37 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
5.29 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.94 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.36 KB, patch
mgaudet
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
6.06 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
6.76 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
2.75 KB, patch
jandem
: review+
Details | Diff | Splinter Review
19.47 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
40.12 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
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.
Flags: needinfo?(jcoppeard)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
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.
Flags: needinfo?(jcoppeard)
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.
(adding CCs for information and bug-avoidance purposes especially for people routinely writing self-hosted JS)
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.
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.
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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 27fd083ed7ee).
(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?
Blocks: 1432479
Flags: needinfo?(jdemooij)
Priority: -- → P1
(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.
Flags: needinfo?(jdemooij)
No longer blocks: 1432479
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?
Flags: needinfo?(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.
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?
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.)
This bug is unassigned, inactive yet it has a test case and a high security rating. Can you help us find an owner, Luke?
Flags: needinfo?(luke)
Jan seems to have a handle on the problem.
Flags: needinfo?(luke) → needinfo?(jdemooij)
sdetar, this is the bug I mentioned earlier this week.
Flags: needinfo?(jdemooij) → needinfo?(sdetar)
Flags: needinfo?(sdetar)
mgaudet is going to look into to the bug and see if he can take it.
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
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 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 ==
Attachment #8965456 - Flags: feedback?(jdemooij) → feedback+
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.
Attachment #8965785 - Flags: review?(jdemooij)
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
Attachment #8965785 - Flags: review?(jdemooij) → feedback+
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.
Attachment #8965785 - Attachment is obsolete: true
Attachment #8967398 - Attachment is obsolete: true
Attachment #8967477 - Flags: review?(jdemooij)
Attachment #8967478 - Flags: review?(jdemooij)
Attachment #8967479 - Flags: review?(jdemooij)
Attachment #8967480 - Flags: review?(jdemooij)
Attachment #8967481 - Flags: review?(jdemooij)
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.
Attachment #8965456 - Attachment is obsolete: true
Attachment #8967478 - Flags: review?(jdemooij) → review+
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 ==
Attachment #8967479 - Flags: review?(jdemooij) → review+
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: ===
Attachment #8967480 - Flags: review?(jdemooij) → review+
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: ===
Attachment #8967481 - Flags: review?(jdemooij) → review+
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)
{

}
Attachment #8967477 - Flags: review?(jdemooij) → review+
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.
Flags: needinfo?(mgaudet)
Attachment #8967477 - Attachment is obsolete: true
Attachment #8967478 - Attachment is obsolete: true
Attachment #8967479 - Attachment is obsolete: true
Attachment #8967480 - Attachment is obsolete: true
Attachment #8967481 - Attachment is obsolete: true
Attached patch Add test case (obsolete) — Splinter Review
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.
Attachment #8969397 - Flags: review?(jdemooij)
Attachment #8969395 - Flags: review+
Attachment #8969394 - Flags: review+
Flags: needinfo?(mgaudet)
Attachment #8969393 - Flags: review+
Attachment #8969392 - Flags: review+
Attachment #8969391 - Flags: review+
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?
Flags: needinfo?(jdemooij)
Comment on attachment 8969397 [details] [diff] [review]
Add test case

(Obsoleting test case patch to avoid landing it)
Attachment #8969397 - Attachment is obsolete: true
Attachment #8969397 - Flags: review?(jdemooij)
(Also, these patches haven't gone through try...)
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.
Flags: needinfo?(tcampbell)
Flags: needinfo?(jdemooij)
OK. I will take a look at that Monday. I can probably get through them, then run some local benchmarking.
(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. :-)
Lays out the ground work
Attachment #8969391 - Attachment is obsolete: true
Attachment #8969392 - Attachment is obsolete: true
Attachment #8969393 - Attachment is obsolete: true
Attachment #8969394 - Attachment is obsolete: true
Attachment #8969395 - Attachment is obsolete: true
Attachment #8970318 - Flags: review?(jdemooij)
Comment on attachment 8970313 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

Carrying :jandem's r+ on Parts 0-4
Attachment #8970313 - Flags: review+
Attachment #8970314 - Flags: review+
Attachment #8970315 - Flags: review+
Attachment #8970316 - Flags: review+
Attachment #8970317 - Flags: review+
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
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
Attachment #8970318 - Flags: review?(jdemooij) → review+
Attachment #8970319 - Flags: review?(jdemooij) → review+
Attachment #8970320 - Flags: review?(jdemooij) → review+
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.
Attachment #8970321 - Flags: review?(jdemooij) → review+
Attachment #8970322 - Flags: review?(jdemooij) → review+
Attachment #8970323 - Flags: review?(jdemooij) → review+
There's also IsSetObject and Is(Shared)ArrayBuffer to convert right?
Attachment #8970322 - Attachment is obsolete: true
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.
Attachment #8970646 - Flags: review+
Attachment #8970647 - Flags: review?(jdemooij)
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
Attachment #8970648 - Flags: review?(jdemooij)
Attachment #8970647 - Flags: review?(jdemooij) → review+
Attachment #8970648 - Flags: review?(jdemooij) → review+
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
Attachment #8970313 - Flags: sec-approval?
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.
Flags: needinfo?(mgaudet)
Attachment #8970313 - Attachment is obsolete: true
Attachment #8970313 - Flags: sec-approval?
Attachment #8970318 - Attachment is obsolete: true
Attachment #8970319 - Attachment is obsolete: true
Comment on attachment 8970968 [details] [diff] [review]
[Part 0] Convert IsStringIterator to GuardToStringIterator

Carrying r+, removed test code.
Attachment #8970968 - Flags: review+
Comment on attachment 8970969 [details] [diff] [review]
[Part 5] Convert IsCollator to GuardToCollator

Carrying r+, edited patch to correct an ESLint error.
Attachment #8970969 - Flags: review+
Comment on attachment 8970970 [details] [diff] [review]
[Part 6] Convert IsNumberFormat to GuardToNumberFormat

Carrying r+, edited patch to fix ESlint error.
Attachment #8970970 - Flags: review+
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)
Flags: needinfo?(mgaudet)
Attachment #8970968 - Flags: sec-approval?
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore], in-testsuite?
What happened to parts 7 and 12?

Is the sec-approval request for the whole bundle or just part 0 standalone?
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?
What about the sec-approval though -- is that a standalone patch? Or are you seeking approval for everything?
Flags: needinfo?(mgaudet)
Seeking approval for the whole stack (first sec-high bug, sorry for this careening off the rails)
Flags: needinfo?(mgaudet)
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
Attachment #8970968 - Flags: sec-approval? → sec-approval+
Comment on attachment 8970981 [details] [diff] [review]
[Part 12] Convert IsSetObject to GuardToSetObject

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

Thanks!
Attachment #8970981 - Flags: review?(jdemooij) → review+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Based on Dan's comments, it sounds like we'll need rebased patches for both ESR60 and ESR52.
Rollup patch for backport.
Rollup patch for backport
Backported patches attached. They've not been tested via try (can we do try builds of older releases?), but do build and run locally.
Flags: needinfo?(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.
Attachment #8972162 - Flags: review+
Attachment #8972162 - Flags: approval-mozilla-esr52?
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.
Attachment #8972163 - Flags: review+
Attachment #8972163 - Flags: approval-mozilla-esr60?
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 on attachment 8972162 [details] [diff] [review]
Change inlining of intrinsics (ESR52)

sec-high fix for 52.9esr
Attachment #8972162 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8972163 [details] [diff] [review]
Change inlining of intrinsics (ESR60)

.. and for 60.1esr
Attachment #8972163 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [jsbugmon:update,ignore], in-testsuite? → [jsbugmon:update,ignore][adv-main61+][adv-esr52.9+][adv-esr60.1+] in-testsuite?
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.