797469 - Assertion failure: addr % Cell::CellSize == 0, at ../../gc/Heap.h:846 or Crash [@ js::gc::MarkInternal]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 13fd49ef7786 (run with ):


gczeal(2);
evaluate('\
var xcode =\
  "\'use strict\';" +\
  "function actX(action) {" +\
  "      try { return eval(\'delete x\'); }" +\
  "      catch (e) { return e.name; }" +\
  "}" +\
  "actX;";\
var f = eval(xcode);\
f("set2");\
',{ 
	global: newGlobal('new-compartment') 
});

Comment 1

[Christian Holler (:decoder)]

Crash trace from opt build:

==6890== Invalid read of size 8
==6890==    at 0x5BCB71: void js::gc::MarkInternal<js::Shape>(JSTracer*, js::Shape**) (Heap.h:1017)
==6890==    by 0x4E9E85: js::Bindings::trace(JSTracer*) (jsscript.cpp:247)
==6890==    by 0x58D6EB: js::frontend::ObjectBox::trace(JSTracer*) (ParseNode.cpp:809)
==6890==    by 0x4648A7: JS::AutoGCRooter::trace(JSTracer*) (jsgc.cpp:2295)
==6890==    by 0x464DBA: JS::AutoGCRooter::traceAll(JSTracer*) (jsgc.cpp:2501)
==6890==    by 0x4683B0: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.325 (jsgc.cpp:2544)
==6890==    by 0x46CC3C: IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3436)
==6890==    by 0x46ED84: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4535)
==6890==    by 0x46FE0E: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4649)
==6890==    by 0x4E4867: _ZN2js2gc10NewGCThingINS_5ShapeEEEPT_P9JSContextNS0_9AllocKindEm.clone.217 (jsgcinlines.h:440)
==6890==    by 0x4E582D: JSObject::toDictionaryMode(JSContext*) (jsgcinlines.h:533)
==6890==    by 0x4E7828: JSObject::addPropertyInternal(JSContext*, long, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>), int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>), unsigned int, unsigned int, unsigned int, int, js::Shape**, bool) (jsscope.cpp:500)
==6890==  Address 0x0 is not stack'd, malloc'd or (recently) free'd


S-s due to GC related crash. This is not necessarily a new bug, but I just fixed a bug in the LangFuzz driver which prevented the evaluate parameters from being used.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This looks parser-related. As far as I can tell, there's an ObjectBox that is not a FunctionBox, but the object it's wrapping is a function. Consequently, we try to trace it (in ObjectBox::trace) as if it's a FunctionBox with bindings even though there are no bindings.

Nick, could you take a look at this? I don't know much about the parser. Might this be a regression from bug 788957?

Comment 3

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   108900:ef321673c843
user:        Eddy Bruel
date:        Tue Oct 02 14:56:26 2012 +0200
summary:     Bug 795721 - Inherit FunctionBox from ObjectBox; r=njn

This iteration took 5.491 seconds to run.

Comment 4

[Christian Holler (:decoder)]

CCing Eddy per comment 3.

Comment 5

[Eddy Bruel [:ejpbruel]]

Yeah, this looks like something I did. Probably some stupid mistake.

I have a day off tomorrow to work on my studies, but I can pick up this bug up this friday. Sorry for the inconvenience.

Comment 6

[Eddy Bruel [:ejpbruel]]

We determine whether an ObjectBox is also a FunctionBox by looking at the type of Object it contains. That is:
objectBox->isFunctionBox() <=> object->isFunction()

This crash suggests that we sometimes store a Function in an ObjectBox rather than a FunctionBox. To my knowledge, we don't ever do that in the parser.

jorendorff, njn, perhaps you could shed some light on this?

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Found the problem. The bytecode compiler was storing a JSFunction in an ObjectBox. This patch fixes that.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 668432 [details] [diff] [review]
Patch to be reviewed

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +151,5 @@
>               * wishes to decompile it while it's running.
>               */
> +            JSFunction *fun = callerFrame->fun();
> +            ObjectBox *funbox = parser.newFunctionBox(fun, &pc, 
> +                                                      fun->inStrictMode() ? StrictMode::STRICT 

delete trailing whitespace

::: js/src/frontend/ParseNode.h
@@ +1461,5 @@
>    public:
>      JSObject *object;
>  
>      ObjectBox(JSObject *object, ObjectBox *traceLink);
> +    ObjectBox(JSFunction *function, ObjectBox *traceLink);

You mentioned making the new constructor protected and I agree.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 668432 [details] [diff] [review]
Patch to be reviewed

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

Great catch!  We were failing to trace the Bindings of these functions, which presumably would lead to the GC reclaiming things it shouldn't.

Comment 10

[Nicholas Nethercote [inactive]]

How long have we been putting this function in an ObjectBox?  You should backport this to Aurora and Beta if necessary.

Comment 11

[Christian Holler (:decoder)]

Can/Shall I land the r+ patch on m-i? This is a blocker for fuzzing.

Comment 12

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

(In reply to Christian Holler (:decoder) from comment #11)
> Can/Shall I land the r+ patch on m-i? This is a blocker for fuzzing.

Sure, but you probably need to make the changes in comment 8 before doing so.

Also, if this misses the upcoming train movement tomorrow, it will probably need to be landed on aurora as well.

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Great catch!  We were failing to trace the Bindings of these functions,
> which presumably would lead to the GC reclaiming things it shouldn't.

That Function object has a reference to its script, which traces the bindings. The reason FunctionBoxes have special tracing needs, I think, is that during parsing, the Function object isn't yet fully hooked up, so it doesn't have a GC-visible reference to the bindings.

That is, AFAIK this isn't the catch you think it is, it's just a recent regression.

This definitely needs to land on Aurora or else the regressing changeset must be backed out.

Comment 14

[bhavana bajaj [:bajaj]]

Can we please have a security rating on this bug ?

Comment 15

[Christian Holler (:decoder)]

As this crash manifests in a lot of ways and it seems that it could lead to a use-after-free condition (esp. according to comment 9), marking this sec-critical. Feel free to correct if this is wrong.

Comment 16

[Eddy Bruel [:ejpbruel]]

Landed on m-c:
https://hg.mozilla.org/integration/mozilla-inbound/rev/467aac9f380a

Comment 17

[Eddy Bruel [:ejpbruel]]

Comment on attachment 668432 [details] [diff] [review]
Patch to be reviewed

[Approval Request Comment]
This patch fixes a regression introduced by bug 795721. That bug is a requirement for harmony modules, so we don't want to back it out. Declining this patch would introduce a crash scenario (see original comment). The fix is rather simple, so the risk of taking the patch is low.

First time I'm doing an approval request, so let me know if above comment is the kind of information you're generally looking for for such requests.

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Christian Holler (:decoder) from comment #15)
> As this crash manifests in a lot of ways and it seems that it could lead to
> a use-after-free condition (esp. according to comment 9), marking this
> sec-critical. Feel free to correct if this is wrong.

See comment 13. Looks like it's not as sec-critical as it may seem.

Comment 19

[Christian Holler (:decoder)]

(In reply to Eddy Bruel [:ejpbruel] from comment #18)

> See comment 13. Looks like it's not as sec-critical as it may seem.

Can you clarify a bit what that the stuff in comment 13 means in terms of security, as it's not completely obvious to me.

Comment 20

[Jason Orendorff [:jorendorff]]

Comment 13 was a reply specifically to comment 9. Comment 9 suggested the bug was old and easy to exploit; but it is very new and quite hard to exploit.

Still, this is sec-critical.

I think we should observe m-c and make sure the fix is good, then land it on mozilla-aurora.

The other option is to back out bug 795721 from mozilla-aurora. Both options are low-risk; the fix is a smaller patch than the backout, and less likely to conflict.

(Eddy: Future work for Harmony modules that depends on the work in bug 795721 won't be affected either way; we're not going to back anything out of m-c, where that future work will happen.)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/467aac9f380a

Comment 24

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 668432 [details] [diff] [review]
Patch to be reviewed

Approving for Auroro as this is sec-critical and tracking it for 19

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I think this needs sec-approval to land on Aurora. Eddy, can you make the request?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Actually, I guess not since it's already on trunk.

Comment 28

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Yeah, the patch is already public at this point, so I don't know how much it matters.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/134f7ce640fd