612481 - Type-punning error in jsemit.cpp:MaybeEmitVarDecl

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

3821     if (JOF_OPTYPE(pn->pn_op) == JOF_LOCAL &&
3822         pn->pn_cookie.slot() < cg->fun->u.i.nvars &&
3823         cg->shouldNoteClosedName(pn))
3824     {
3825         if (!cg->closedVars.append(pn->pn_cookie.slot()))
3826             return JS_FALSE;
3827     }

Nothing guarantees that we're in a function, so the reference to cg->fun is misguided.  Here's a simple testcase which hits this with !cg->inFunction():

  for (let j = 0; j < 5; j++);

The result is that we pun cg->scopeChain as a JSFunction*.  On my 64-bit system, this results in our punning past the end of cg->scopeChain (the global object) to read a uint16_t at 96 bytes from the start of a 72-byte object.  The 24 bytes we run past the end drags us into the next object, which happens to be the regexp_statics_class object for the global object (see JS_NewGlobalObject).  This appears to always be the case: JSCLASS_GLOBAL_FLAGS currently implies 123 reserved slots, which well overflows what our variable-length objects support, so we give up and create an object with no fixed slots.  The regexp_statics_class encodes no reserved slots.  So these two objects will always be of the same GC kind.  And the regex class produces an object with an emptyShapes field (at offset 24) of NULL, so the nvars reference above is 0.

I *think* the effect of this (on 64-bit) is that the vast majority of the time you're going to read from the regexp_statics_class object and get a 0, so nothing untoward happens.  But if you get unlucky, and your global object is allocated at the end of an arena, and your regexp object is allocated in a different one, you'll read off the end and...something unpredictable will happen.  And I don't know how the offsets work out if you're on a non-64-bit system.  So there may still be a problem here even beyond the once-in-a-blue-moon case of hitting an arena boundary just after allocating a global object and before allocating the statics object.  Beyond that, the regexp_statics_class object is relatively new -- so I don't know what happens in older code, or older branches.

I think we can solve this with a inFunction() guard, but I'm not quite certain.

Is there some reason fun/scopeChain are overlaid, public, and not debug-checked at runtime for consistency with cg->inFunction()?  I caught this with a patch implementing precisely such accessors with assertions (in the middle of a queue, not easily posted just now, but the patch is simple enough).  It would seem like a good rule to me that unions, whenever we use them (outside JITs, alas), must be private and accessed only through correctness-asserting accessors.

Comment 1

[Brendan Eich [:brendan]]

This just needs an inFunction condition.

The unioning could be undone, and we'd have a null deref. Since we had a flag bit as discriminant, we unioned a while ago. I don't see other bugs like this one, which is fairly recent. David should review.

/be

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 3

[Jeff Walden [:Waldo]]

This code's new as of September-ish, so it turns out there are no worries about branches.  I'll keep this closed until it makes its way into a beta, just to keep people using betas marginally safer in the meantime.

On a lark I checked crash reports, and of the four crash reports for MaybeEmitVarDecl (from what time range?) it looks like one may be this crash:

http://crash-stats.mozilla.com/report/index/fe777df6-7200-4663-86f9-9415b2101115

(In reply to comment #1)
> The unioning could be undone, and we'd have a null deref.

Unions are awesome.  :-)  It's accessing them without asserting correct discrimination that gets me, a problem easily avoided by never accessing the raw field except through accessors that make it impossible to forget the assertions.  I'll see if I can splice out that patch to use accessors here, probably won't be too bad.

Comment 4

[Daniel Veditz [:dveditz]]

Could the extra slots contain things that might be interpreted as something to execute? Assuming worst case sg:critical. If it's just simple data that might be someone else's data maybe sg:moderate or sg:high would be more appropriate.

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/9baf4ce0920b

I don't know what's past the end; 64-bit debug it's a guaranteed-zero field in a known object, but 64-bit opt, 32-bit debug, 32-bit opt it might (or might not) be some other field.  And if you reach the end of an arena after allocating the global but before allocating the statics object, you're reading off the end of the arena, and I don't know what's there -- might be another arena, or perhaps just random heap, I'm not familiar with how the GC allocates arenas these days to say.  I also don't know the full danger of what happens if that condition is true -- someone really trying to track this down would have to check into that.

But since this is trunk-only, easily fixed, and difficult to hit, I probably have better things to do than conduct a mostly academic investigation into exactly what could go wrong in every possible case.

Comment 6

[Jeff Walden [:Waldo]]

The push that included this change also included a change for bug 612675 to add those accessors, but I forgot to note the bug number in the changeset message -- hopefully this is enough of a breadcrumb for anyone trying to do the archaeology.  :-(

Comment 7

[Brendan Eich [:brendan]]

Good point, the union needs C++ inline getters that assert. That was not done when the union came in, after our switch to C++, so no good "C made me do it" excuse. Thanks for catching this.

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9baf4ce0920b