Assertion failure: !has(reg), at js/src/jit/RegisterSets.h:860

RESOLVED FIXED in Firefox 47

Status

()

--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nbp, Assigned: jandem)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla47
ARM
Linux
assertion, crash, regression, sec-want, testcase
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox47 fixed)

Details

(Whiteboard: [jsbugmon:update][fuzzblocker][adv-main47-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
The following testcase crashes on mozilla-central revision e7d613b3bcfe (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-simulator=arm --enable-debug, run with --arm-hwcap=vfp --baseline-eager min.js):

a = new Array;
for (var i = 0; i != 1000; -- i) a[i] = 17;



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0869a6fd in add (reg=..., this=0xffffc85c) at js/src/jit/RegisterSets.h:860
#0  0x0869a6fd in add (reg=..., this=0xffffc85c) at js/src/jit/RegisterSets.h:860
#1  js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope (this=0xffffbf18, masm=..., reg=...) at js/src/jit/MacroAssembler.cpp:2649
#2  0x0871b268 in ScratchRegisterScope (masm=..., this=0xffffbf18) at js/src/jit/arm/Assembler-arm.h:54
#3  js::jit::MacroAssemblerARM::ma_alu (this=0xffffc13c, src1=..., imm=..., dest=..., op=js::jit::OpMov, s=js::jit::LeaveCC, c=js::jit::Assembler::Always) at js/src/jit/arm/MacroAssembler-arm.cpp:417
#4  0x0871ba0b in js::jit::MacroAssemblerARM::ma_mov (this=this@entry=0xffffc13c, imm=..., dest=..., s=s@entry=js::jit::LeaveCC, c=c@entry=js::jit::Assembler::Always) at js/src/jit/arm/MacroAssembler-arm.cpp:527
#5  0x08223d79 in js::jit::MacroAssemblerARMCompat::storeValue (this=0xffffc13c, val=..., dest=...) at js/src/jit/arm/MacroAssembler-arm.h:1138
#6  0x085f472d in storeValue (dest=..., val=..., this=0xffffc13c) at js/src/jit/arm/MacroAssembler-arm.h:1149
#7  StoreDenseElement (target=..., elements=..., value=..., masm=...) at js/src/jit/IonCaches.cpp:4173
#8  GenerateSetDenseElement (temp=..., value=..., indexVal=..., object=..., guardHoles=<optimized out>, idval=..., obj=<optimized out>, attacher=..., masm=..., cx=<optimized out>, tempToUnboxIndex=...) at js/src/jit/IonCaches.cpp:4291
#9  js::jit::SetElementIC::attachDenseElement (this=this@entry=0xf7a39924, cx=cx@entry=0xf7a7f020, outerScript=outerScript@entry=..., ion=ion@entry=0xf7a39800, obj=obj@entry=..., idval=...) at js/src/jit/IonCaches.cpp:4312
#10 0x085f5521 in js::jit::SetElementIC::update (cx=cx@entry=0xf7a7f020, outerScript=..., outerScript@entry=..., cacheIndex=cacheIndex@entry=100, obj=..., obj@entry=..., idval=..., idval@entry=..., value=value@entry=...) at js/src/jit/IonCaches.cpp:4433
[…]
eax	0x0	0
ebx	0x977a95c	158837084
ecx	0xf7e4388c	-136038260
edx	0x0	0
esi	0x5000	20480
edi	0x1e00000	31457280
ebp	0xffffbeb8	4294950584
esp	0xffffbea0	4294950560
eip	0x869a6fd <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+93>
=> 0x869a6fd <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+93>:	movl   $0x35c,0x0
   0x869a707 <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+103>:	call   0x80f2740 <abort()>
(Reporter)

Updated

3 years ago
Flags: needinfo?(sstangl)
Flags: needinfo?(nicolas.b.pierron)
This one is a false positive and a bug (and not S-S).

The ScratchRegister is taken by storeValue() and passed as the dest operand of ma_alu(), which calls another ma_alu() with a different scratch register. We have a check in ma_alu() to ignore the case when the ScratchRegister is used as the destination, but the extra level of indirection knocks out the detection.

I would suggest making those anti-scope scopes to knock out the ScratchRegister when it is being used as the destination argument. That would eliminate the weird logic inside ma_alu(). I'll do that in a bit.
It turns out that this is actually S-S. If you fix the first bug, another one crops up where the address to which a write is performed is overwritten. It looks like the address could plausibly be user-controllable.
Oh, no, it is actually fine. In the latter case that I thought was a bug, we are !HasMOVWT() and op == OpMov, so we wind up snaking through a codepath that uses a constant pool load into the dest register. So the scratch register is not overwritten and it is again not S-S.

This code is pretty inscrutable.
Created attachment 8665102 [details] [diff] [review]
0001-Bug-1206652-Use-a-destination-only-scratch-register-.patch

This addresses this particular failure mode. Note that we cannot simply use dest as a scratch register, since apparently sometimes dest == src1 in ma_alu().

Still think all this code needs to be rewritten.
Flags: needinfo?(sstangl)
Attachment #8665102 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8665102 [details] [diff] [review]
0001-Bug-1206652-Use-a-destination-only-scratch-register-.patch

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

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1136,1 @@
>          AutoRegisterScope scratch2(asMasm(), secondScratchReg_);

I do not see the point of having an additional class and assertion with AutoDestRegisterScope, as the assertion of AutoRegisterScope will trigger the same way.

I think what we are interested in is some way of saying that we aquire the scratch register at the end of the call.  Thus, I do not think we can solve this issue with an argument, but I do think we can solve that with an lvalue.

  AutoScratchRegister scratch(asMasm(), scratchReg_, /* acquired */ false);
  ma_mov(ImmTag(…), scratch);
  // scratch.acquire()

This solution is not that nice, as it does not fit with our way to write the macro assembler code, and because it is easy to forget the acquire call, but I think this would be good enough for this bug.

Another solution would be to have value scopes, such that each instruction mention that a new value is pushed into a register, and have a stack-based list of ValueScope, which have either one unique color or no color.

  ValueScope scratch(asMasm(), ScratchReg);
  ma_mov(ImmTag(…), scratch);

and then inside the ma_mov() function we unconditionally "color" the ValueScope associated with the last destination register, when it is set.

Any attempt to re-color a ValueScope (do another set) or to destroy a ValueScope while the color is different than the parent will cause an assertion.

Basically, the color is just an abstract way to identify values, almost as we do with a register allocator.  The difference with the current AutoRegisterScope, is that we can potentially re-use the same register while using a temp register/memory location to hold the "color" of a value.
Attachment #8665102 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> I do not see the point of having an additional class and assertion with
> AutoDestRegisterScope, as the assertion of AutoRegisterScope will trigger
> the same way.

It will not. If you read the patch carefully, the AutoDestRegisterScope asserts on entry, but does not set the register ownership bit. Therefore it is possible to independently acquire the ScratchRegister in the callee.

> I think what we are interested in is some way of saying that we aquire the
> scratch register at the end of the call.  Thus, I do not think we can solve
> this issue with an argument, but I do think we can solve that with an lvalue.
> 
>   AutoScratchRegister scratch(asMasm(), scratchReg_, /* acquired */ false);
>   ma_mov(ImmTag(…), scratch);
>   // scratch.acquire()

I strongly prefer the DestScope solution to more stateful tracking.

> This solution is not that nice, as it does not fit with our way to write the
> macro assembler code, and because it is easy to forget the acquire call, but
> I think this would be good enough for this bug.
> 
> Another solution would be to have value scopes, such that each instruction
> mention that a new value is pushed into a register, and have a stack-based
> list of ValueScope, which have either one unique color or no color.

DestScope is much simpler than independent color tracking, which way overcomplicates the issue. If you read the implementation again with the above understanding, I think you will like it more than these other suggestions. Please let me know.
(Reporter)

Comment 7

3 years ago
(In reply to Sean Stangl [:sstangl] from comment #6)
> (In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > I do not see the point of having an additional class and assertion with
> > AutoDestRegisterScope, as the assertion of AutoRegisterScope will trigger
> > the same way.
> 
> It will not. If you read the patch carefully, the AutoDestRegisterScope
> asserts on entry, but does not set the register ownership bit. Therefore it
> is possible to independently acquire the ScratchRegister in the callee.

I do understand that, and I think I read the patch carefully.

Still, I think the AutoDestRegisterScope is still useless, as it is followed by an AutoRegisterScope which asserts the same way.

These Auto*Scope are only use to assert that no enclosing caller already has the ownership of the register.  Thus the AutoDestRegisterScope does not add any value, as it checks that no caller own the register, which is identical to what AutoRegisterScope does.  This is why I think that AutoDestRegisterScope does not bring any value, if followed by an AutoRegisterScope of the same register, which is more than likely.

What we are looking for is a way to assert that the returned value is not erased before being used. (use-after-scratch) or used after leaving the scope (use-after-free).
(Reporter)

Comment 8

3 years ago
Created attachment 8677547 [details] [diff] [review]
Draft of AsmInput / AsmOutput / AsmInOut classes.

This patch does not do anything except adding a few no-op classes.

These no-op classes are so far wrapping a RegisterType while they could be
useful for annotating function declarations to make a bit more
understandable for novice.

The advantage of these classes is that we could remove the inheritance on
RegisterType and make the cast to registers explicit.  Such explicit case
would be extremely useful if we were to add a new LiveRegisterSet for bound
registers.

In which case we could have the following rules:

  RegisterType AsmInput::reg() const {
    MOZ_ASSERT_IF(scope_, scope_->isDefined())
    return reg_;
  }

  RegisterType AsmInOut::reg() const {
    MOZ_ASSERT_IF(scope_, scope_->isDefined())
    return reg_;
  }

  RegisterType AsmOutput::reg() const {
    setWithDestructor_ = true;
    return reg_;
  }

  AsmOutput::~AsmOutput() {
      if (setWithDestructor_ && scope_)
          scope_->define(reg_)
  }

This way, the scope is just the class which provide access to the bounded
flag, and the ownership of the bound flag belongs to the AsmScope, which
means that one inner functions could own the same register if the register
was not bound with the enclosing scope.

I would also say, that even if we don't want to add assertions into these
AsmInput / AsmOutput and AsmInOut, these are quite interesting just for the
readability aspect of the Assembler / MacroAssembler code.
Attachment #8677547 - Flags: feedback?(sstangl)
(Reporter)

Updated

3 years ago
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8677547 [details] [diff] [review]
Draft of AsmInput / AsmOutput / AsmInOut classes.

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

I really don't understand this patch. Instead of a class that relinquishes scratch register scope to the callee, as with the previous patch, this one seems to be suggesting annotating all MacroAssembler function arguments with classes that specify whether the argument is an input, an output, or an input/output. That still doesn't clearly map to the case where the callee takes ownership of the input, then doesn't return it. It also seems like a lot of work to provide these annotations all over the MacroAssembler.

It looks like we're trying to shoehorn the Rust borrow system into C++, which won't work. I would strongly suggest just taking the previous patch, which is clumsy, but minimal and good enough for what we're trying to accomplish here.
Attachment #8677547 - Flags: feedback?(sstangl)
I would also suggest that since we have a difference of opinion on how far to go to fix this problem, that we ask Jan to decide, rather than having a long Bugzilla thread.

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 11

3 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Updated

3 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Whiteboard: [jsbugmon:] → [jsbugmon:update]

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:]

Comment 12

3 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
(In reply to Sean Stangl [:sstangl] from comment #10)
> I would also suggest that since we have a difference of opinion on how far
> to go to fix this problem, that we ask Jan to decide, rather than having a
> long Bugzilla thread.

Setting needinfo? from Jan based on this comment.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 14

3 years ago
(In reply to Sean Stangl [:sstangl] from comment #10)
> I would also suggest that since we have a difference of opinion on how far
> to go to fix this problem, that we ask Jan to decide, rather than having a
> long Bugzilla thread.

Hm. I don't have a strong opinion on it, but I agree with comment 9 that annotating the register arguments to all MacroAssembler methods seems overkill to fix this bug.

Is it possible to rewrite/restructure the problematic ARM code so we don't need any new classes?
Flags: needinfo?(jdemooij) → needinfo?(sstangl)
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][fuzzblocker]
(Assignee)

Comment 15

3 years ago
I'm working on patches to disentangle ma_mov and ma_alu.

The new ma_mov is much simpler than ma_alu, and does not require a scratch register anywhere. Hopefully with that we don't need any new classes, and we can simplify the scratch register situation in some of the callers as well.
Flags: needinfo?(sstangl) → needinfo?(jdemooij)
(Assignee)

Comment 16

3 years ago
Created attachment 8713071 [details] [diff] [review]
Part 1 - Remove ma_mov's SBit argument

This patch removes the SBit argument from the ma_mov methods that move a constant into a register.

Setting condition codes in this case doesn't make a lot of sense: because it's a constant, we know at compile-time which flags will be set, and all callers passed LeaveCC.

It also removes an unused ma_mvn method.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8713071 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 17

3 years ago
Created attachment 8713072 [details] [diff] [review]
Part 2 - Disentangle ma_mov and ma_alu

This patch reimplements ma_mov separately from ma_alu. As a result, both of these are much simpler now than the original ma_alu was.

 1 files changed, 43 insertions(+), 81 deletions(-)
Attachment #8665102 - Attachment is obsolete: true
Attachment #8677547 - Attachment is obsolete: true
Attachment #8713072 - Flags: review?(nicolas.b.pierron)
(Reporter)

Updated

3 years ago
Attachment #8713071 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Comment 18

3 years ago
Comment on attachment 8713072 [details] [diff] [review]
Part 2 - Disentangle ma_mov and ma_alu

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

Nice code improvement!
Attachment #8713072 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 19

3 years ago
Agreed with comment 3 that this is not s-s.
Group: javascript-core-security

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/180ed3d82d77
https://hg.mozilla.org/mozilla-central/rev/e67b6264ef64
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Whiteboard: [jsbugmon:update][fuzzblocker] → [jsbugmon:update][fuzzblocker][adv-main47-]
You need to log in before you can comment on or make changes to this bug.