Closed Bug 701332 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla11
Tracking Status
firefox8 - unaffected
firefox9 + unaffected
firefox10 + fixed
firefox11 + verified
firefox12 --- fixed
status1.9.2 --- unaffected

People

(Reporter: decoder, Assigned: cdleary)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical][qa+])

Attachments

(2 files)

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).
Fix coming up, plus a way to avoid this category of errors going forward. :-P
Assignee: general → cdleary
Whiteboard: [sg:critical] js-triage-needed → [sg:critical]
Attached patch Minimal fix.Splinter Review
Smallest fix.
Attachment #573579 - Flags: review?(jwalden+bmo)
Hide RegExpPrivates from the outside world by use of an auto-refcounting matcher interface.
Attachment #573580 - Flags: review?(jwalden+bmo)
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.
Attachment #573580 - Flags: review?(jwalden+bmo) → review+
Attachment #573579 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 573579 [details] [diff] [review]
Minimal fix.

10 is affected.
Attachment #573579 - Flags: approval-mozilla-aurora?
Comment on attachment 573579 [details] [diff] [review]
Minimal fix.

Approving for Aurora (10)

Do we know for sure that Beta (9) is affected?
Attachment #573579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Has this landed on Aurora? Do we know if FF9 is affected?
Whiteboard: [sg:critical] → [sg:critical][qa+]
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?
Just to double check, this is fixed in Firefox 11 given that the bug is marked as fixed?
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?
(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.
Blocks: 634654
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][regression]
Keywords: regression
Whiteboard: [sg:critical][qa+][regression] → [sg:critical][qa+]
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?
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
Group: core-security
Status: RESOLVED → VERIFIED
Slow GC test, not taking for the test suite.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.