701332 - SIGTRAP on heap (in JSC::Yarr::execute code) with use after free

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision c60535115ea1 (options -m -n):


gczeal(2);
test();
function test() {
  function t(o, proplist) {
    var props=proplist.split(/\s+/g);
  };
  t({ bar: 123, baz: 123, quux: 123 }, 'bar baz quux');
  ( test(), this )(( "7.4.2-6-n" ) , actual, summary + ' : nonjit');
}

Backtrace from valgrind:

==30698== Invalid read of size 4
==30698==    at 0x651B7C: js::RegExpPrivate::pairCount() const (RegExpObject.h:357)
==30698==    by 0x64FDCE: js::RegExpPrivate::execute(JSContext*, unsigned short const*, unsigned long, unsigned long*, js::LifoAllocScope&, js::MatchPairs**) (RegExpObject.cpp:191)
==30698==    by 0x6581B0: bool ExecuteRegExpImpl<js::RegExpPrivate>(JSContext*, js::RegExpStatics*, js::RegExpPrivate*, JSLinearString*, unsigned short const*, unsigned long, unsig
ned long*, js::RegExpExecType, JS::Value*) (RegExp.cpp:135)
==30698==    by 0x655B8A: js::ExecuteRegExp(JSContext*, js::RegExpStatics*, js::RegExpPrivate*, JSLinearString*, unsigned short const*, unsigned long, unsigned long*, js::RegExpExe
cType, JS::Value*) (RegExp.cpp:166)
==30698==    by 0x5921B2: SplitRegExpMatcher::operator()(JSContext*, JSLinearString*, unsigned long, SplitMatchResult*) (jsstr.cpp:2425)
==30698==    by 0x58E98E: JSObject* SplitHelper<SplitRegExpMatcher>(JSContext*, JSLinearString*, unsigned int, SplitRegExpMatcher, js::types::TypeObject*) (jsstr.cpp:2313)
==30698==    by 0x589BF0: js::str_split(JSContext*, unsigned int, JS::Value*) (jsstr.cpp:2536)
==30698==    by 0x4FF690: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:297)
==30698==    by 0x4E0D62: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:629)
==30698==    by 0x4F0D87: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:3948)
==30698==    by 0x4E0B16: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:584)
==30698==    by 0x4E15F8: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:783)
==30698==  Address 0x5e38658 is 56 bytes inside a block of size 64 free'd
==30698==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==30698==    by 0x40382B: js_free (Utility.h:183)
==30698==    by 0x4114F3: js::Foreground::free_(void*) (Utility.h:597)
==30698==    by 0x41A3D3: JSRuntime::free_(void*) (jscntxt.h:770)
==30698==    by 0x41A4B6: JSContext::free_(void*) (jscntxt.h:1269)
==30698==    by 0x593883: void JSContext::delete_<js::RegExpPrivate>(js::RegExpPrivate*) (in /srv/repos/mozilla-central/js/src/debug64/shell/js)
==30698==    by 0x5915BA: js::RegExpPrivate::decref(JSContext*) (RegExpObject-inl.h:372)
==30698==    by 0x651C39: js::RegExpObject::purge(JSContext*) (RegExpObject-inl.h:132)
==30698==    by 0x651C70: js::RegExpObject::finalize(JSContext*) (RegExpObject-inl.h:140)
==30698==    by 0x65041A: regexp_finalize(JSContext*, JSObject*) (RegExpObject.cpp:336)
==30698==    by 0x4ACF12: JSObject::finalize(JSContext*) (jsobjinlines.h:267)
==30698==    by 0x4B0C69: bool js::gc::Arena::finalize<JSObject>(JSContext*, js::gc::AllocKind, unsigned long) (jsgc.cpp:302)

[ More invalid reads here ]

==30698== Process terminating with default action of signal 5 (SIGTRAP)
==30698==    at 0x40392B9: ???
==30698==    by 0x7242C2: JSC::Yarr::execute(JSC::Yarr::YarrCodeBlock&, unsigned short const*, unsigned int, unsigned int, int*) (YarrJIT.cpp:2461)
==30698==    by 0x65288C: js::RegExpPrivateCode::execute(JSContext*, unsigned short const*, unsigned long, unsigned long, int*, unsigned long) (RegExpObject-inl.h:334)
==30698==    by 0x64FE9B: js::RegExpPrivate::execute(JSContext*, unsigned short const*, unsigned long, unsigned long*, js::LifoAllocScope&, js::MatchPairs**) (RegExpObject.cpp:212)
==30698==    by 0x6581B0: bool ExecuteRegExpImpl<js::RegExpPrivate>(JSContext*, js::RegExpStatics*, js::RegExpPrivate*, JSLinearString*, unsigned short const*, unsigned long, unsigned long*, js::RegExpExecType, JS::Value*) (RegExp.cpp:135)
==30698==    by 0x655B8A: js::ExecuteRegExp(JSContext*, js::RegExpStatics*, js::RegExpPrivate*, JSLinearString*, unsigned short const*, unsigned long, unsigned long*, js::RegExpExecType, JS::Value*) (RegExp.cpp:166)
==30698==    by 0x5921B2: SplitRegExpMatcher::operator()(JSContext*, JSLinearString*, unsigned long, SplitMatchResult*) (jsstr.cpp:2425)
==30698==    by 0x58E98E: JSObject* SplitHelper<SplitRegExpMatcher>(JSContext*, JSLinearString*, unsigned int, SplitRegExpMatcher, js::types::TypeObject*) (jsstr.cpp:2313)
==30698==    by 0x589BF0: js::str_split(JSContext*, unsigned int, JS::Value*) (jsstr.cpp:2536)
==30698==    by 0x4FF690: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:297)
==30698==    by 0x4E0D62: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:629)
==30698==    by 0x4F0D87: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:3948)


S-s and sg:critical due to use-after-free and SIGTRAP (usually indicating that execution hit a TRAP slide which would lead to execution of random memory in optimized builds).

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Fix coming up, plus a way to avoid this category of errors going forward. :-P

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Minimal fix.

Smallest fix.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Hide RegExpPrivates.

Hide RegExpPrivates from the outside world by use of an auto-refcounting matcher interface.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 573580 [details] [diff] [review]
Hide RegExpPrivates.

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

I am a fan of not exposing RegExpPrivate.

::: js/src/builtin/RegExp.cpp
@@ +158,5 @@
>      return CreateRegExpMatchResult(cx, input, chars, length, matchPairs, rval);
>  }
>  
>  bool
> +js::ExecuteRegExp(JSContext *cx, RegExpStatics *res, RegExpMatcher *matcher, JSLinearString *input,

|matcher| should be a reference.

::: js/src/builtin/RegExp.h
@@ +63,5 @@
>                const jschar *chars, size_t length,
>                size_t *lastIndex, RegExpExecType type, Value *rval);
>  
>  bool
> +ExecuteRegExp(JSContext *cx, RegExpStatics *res, RegExpMatcher *matcher, JSLinearString *input,

Reference.

::: js/src/jsstr.cpp
@@ +1295,4 @@
>  
>      RegExpObject *reobj() const { return reobj_; }
>  
> +    RegExpMatcher *getMatcher() const {

This should return a reference and be matcher().

@@ +2401,5 @@
>   * index at which a match happens.
>   */
>  class SplitRegExpMatcher {
>      RegExpStatics *res;
> +    RegExpMatcher *matcher;

Reference.

::: js/src/vm/RegExpObject-inl.h
@@ +394,5 @@
>  
> +#ifdef DEBUG
> +    this->~RegExpPrivate();
> +    memset(this, 0xcd, sizeof(*this));
> +    cx->free_(this);

We should have poisoningFree and poisoningDelete methods, that poison in debug builds.  Or something.  I dunno.  This is fine for now.

::: js/src/vm/RegExpObject.h
@@ +162,5 @@
>      inline void purge(JSContext *x);
>  
> +    /*
> +     * Trigger an eager creation of the associated RegExpPrivate. Note
> +     * that a GC may purge it away.

"away" feels redundant to me, maybe.

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16d8593b432a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78a8eaa8a7b

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74b1322c287e (followup fix)

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/mozilla-central/rev/16d8593b432a
https://hg.mozilla.org/mozilla-central/rev/c78a8eaa8a7b
https://hg.mozilla.org/mozilla-central/rev/74b1322c287e

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 573579 [details] [diff] [review]
Minimal fix.

10 is affected.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 573579 [details] [diff] [review]
Minimal fix.

Approving for Aurora (10)

Do we know for sure that Beta (9) is affected?

Comment 10

[Alex Keybl [:akeybl]]

Has this landed on Aurora? Do we know if FF9 is affected?

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Sorry, I failed to update this bug appropriately -- the backout from

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=3f725329f26d

reverted lazy regexps, so Aurora is no longer affected. All extant RegExp objects hold a refcount to an eagerly-created RegExpPrivate and the RegExp object in str_split is appropriately rooted in vp[2].

Waldo, do you mind double checking me?

Comment 12

[Johnny Stenback (:jst)]

Just to double check, this is fixed in Firefox 11 given that the bug is marked as fixed?

Comment 13

[Daniel Veditz [:dveditz]]

Chris: comment 11 seems to be saying this bug is a regression from something, which was then backed out from Fx10 Aurora? If so please add the regression keyword and the bug number to the "Blocks" field. I think you're also saying we should set the  firefox 11 status field to "fixed" and for firefox 10 to either fixed or unaffected.

What about 9?

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Daniel Veditz from comment #13)
> Chris: comment 11 seems to be saying this bug is a regression from
> something, which was then backed out from Fx10 Aurora? If so please add the
> regression keyword and the bug number to the "Blocks" field.

Done! For reference, the Fx10 Aurora backout was performed in bug 702426.

> I think you're
> also saying we should set the  firefox 11 status field to "fixed" and for
> firefox 10 to either fixed or unaffected.

Fx11 is fixed by the trunk changesets given above, and Fx10 Aurora is unaffected due to the backout that I was referencing.

> What about 9?

Fx9 Beta was unaffected.

Comment 15

[u279076]

I'm trying to verify this with the Firefox 11.0b6 js-shell by copying in the following code:

gczeal(2);
test();
function test() {
  function t(o, proplist) {
    var props=proplist.split(/\s+/g);
  };
  t({ bar: 123, baz: 123, quux: 123 }, 'bar baz quux');
  ( test(), this )(( "7.4.2-6-n" ) , actual, summary + ' : nonjit');
}

I get the following output:
typein:1: ReferenceError: gczeal is not defined
typein:2: ReferenceError: test is not defined

Am I doing something wrong?

Comment 16

[u279076]

I got my problem sorted out -- built a debug shell from today's mozilla-beta and confirmed the test does not crash.

Verified for Firefox 11.0b6

Comment 17

[Christian Holler (:decoder)]

Slow GC test, not taking for the test suite.