818604 - Assertion failure: !entered && i < mLength, at ./dist/include/js/Vector.h:346 or Invalid read [@ JSC::Yarr::YarrPatternConstructor::containsCapturingTerms]

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 6c23f41b0747 (run with --ion-eager -a):


testRegexp("$.+$", "gi");
testRegexp(".*(?:)bc$", "gi");
testRegexp(".*(?:)bc$$.+$", "gi");
function testRegexp(res, mode) {
    re = new RegExp(res, mode);
}
var re2 = new RegExp(re);

Comment 1

[Christian Holler (:decoder)]

Caught by the RegExp module in LangFuzz. In optimized builds, this test doesn't crash but it shows invalid reads in Valgrind:


==43274== Conditional jump or move depends on uninitialised value(s)
==43274==    at 0x7CCF8A: JSC::Yarr::YarrPatternConstructor::containsCapturingTerms(JSC::Yarr::PatternAlternative*, unsigned long, unsigned long) (YarrPattern.cpp:764)
==43274==    by 0x7D3F92: JSC::Yarr::YarrPattern::compile(JSLinearString const&) (YarrPattern.cpp:821)
==43274==    by 0x7D4284: JSC::Yarr::YarrPattern::YarrPattern(JSLinearString const&, bool, bool, JSC::Yarr::ErrorCode*) (YarrPattern.cpp:892)
==43274==    by 0x64166B: js::detail::RegExpCode::compile(JSContext*, JSLinearString&, unsigned int*, js::RegExpFlag) (RegExpObject.cpp:178)
==43274==    by 0x6440D8: js::RegExpShared::compile(JSContext*, JSAtom*) (RegExpObject.cpp:453)
==43274==    by 0x645930: js::RegExpObject::createShared(JSContext*, js::RegExpGuard*) (RegExpObject.cpp:582)
==43274==    by 0x64BFA4: _ZL19CompileRegExpObjectP9JSContextRN2js19RegExpObjectBuilderEN2JS8CallArgsE.isra.131.constprop.133 (RegExpObject-inl.h:50)
==43274==    by 0x64C030: regexp_construct(JSContext*, unsigned int, JS::Value*) (RegExp.cpp:341)
==43274==    by 0x4BB731: js::InvokeConstructorKernel(JSContext*, JS::CallArgs) (jscntxtinlines.h:364)
==43274==    by 0x4ACE99: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2331)
==43274==    by 0x660887: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1070)
==43274==    by 0x660ABB: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1101)
==43274==
==43274== Invalid read of size 1
==43274==    at 0x7CCF7D: JSC::Yarr::YarrPatternConstructor::containsCapturingTerms(JSC::Yarr::PatternAlternative*, unsigned long, unsigned long) (YarrPattern.cpp:761)
==43274==    by 0x7D3F92: JSC::Yarr::YarrPattern::compile(JSLinearString const&) (YarrPattern.cpp:821)
==43274==    by 0x7D4284: JSC::Yarr::YarrPattern::YarrPattern(JSLinearString const&, bool, bool, JSC::Yarr::ErrorCode*) (YarrPattern.cpp:892)
==43274==    by 0x64166B: js::detail::RegExpCode::compile(JSContext*, JSLinearString&, unsigned int*, js::RegExpFlag) (RegExpObject.cpp:178)
==43274==    by 0x6440D8: js::RegExpShared::compile(JSContext*, JSAtom*) (RegExpObject.cpp:453)
==43274==    by 0x645930: js::RegExpObject::createShared(JSContext*, js::RegExpGuard*) (RegExpObject.cpp:582)
==43274==    by 0x64BFA4: _ZL19CompileRegExpObjectP9JSContextRN2js19RegExpObjectBuilderEN2JS8CallArgsE.isra.131.constprop.133 (RegExpObject-inl.h:50)
==43274==    by 0x64C030: regexp_construct(JSContext*, unsigned int, JS::Value*) (RegExp.cpp:341)
==43274==    by 0x4BB731: js::InvokeConstructorKernel(JSContext*, JS::CallArgs) (jscntxtinlines.h:364)
==43274==    by 0x4ACE99: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2331)
==43274==    by 0x660887: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1070)
==43274==    by 0x660ABB: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1101)
==43274==  Address 0x5f48e0c is not stack'd, malloc'd or (recently) free'd
==43274==
==43274== Invalid read of size 4
==43274==    at 0x7CCF87: JSC::Yarr::YarrPatternConstructor::containsCapturingTerms(JSC::Yarr::PatternAlternative*, unsigned long, unsigned long) (YarrPattern.cpp:764)
==43274==    by 0x7D3F92: JSC::Yarr::YarrPattern::compile(JSLinearString const&) (YarrPattern.cpp:821)
==43274==    by 0x7D4284: JSC::Yarr::YarrPattern::YarrPattern(JSLinearString const&, bool, bool, JSC::Yarr::ErrorCode*) (YarrPattern.cpp:892)
==43274==    by 0x64166B: js::detail::RegExpCode::compile(JSContext*, JSLinearString&, unsigned int*, js::RegExpFlag) (RegExpObject.cpp:178)
==43274==    by 0x6440D8: js::RegExpShared::compile(JSContext*, JSAtom*) (RegExpObject.cpp:453)
==43274==    by 0x645930: js::RegExpObject::createShared(JSContext*, js::RegExpGuard*) (RegExpObject.cpp:582)
==43274==    by 0x64BFA4: _ZL19CompileRegExpObjectP9JSContextRN2js19RegExpObjectBuilderEN2JS8CallArgsE.isra.131.constprop.133 (RegExpObject-inl.h:50)
==43274==    by 0x64C030: regexp_construct(JSContext*, unsigned int, JS::Value*) (RegExp.cpp:341)
==43274==    by 0x4BB731: js::InvokeConstructorKernel(JSContext*, JS::CallArgs) (jscntxtinlines.h:364)
==43274==    by 0x4ACE99: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2331)
==43274==    by 0x660887: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1070)
==43274==    by 0x660ABB: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1101)
==43274==  Address 0x5f48e08 is not stack'd, malloc'd or (recently) free'd


The options specified in comment 0 aren't necessary, it also reproduces without any options, even with --no-ion which is why it's unlikely to be an IonMonkey regression. S-s due to invalid reads, assuming sec-high at least.

Comment 2

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   112142:8bf2f8cb5e73
user:        David Anderson
date:        Thu Nov 01 21:35:25 2012 -0700
summary:     Update Yarr to WebKit rev 130234 (bug 740015, r=dmandelin).

This iteration took 67.805 seconds to run.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision c8a1314aa449).

Comment 4

[Christian Holler (:decoder)]

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

The first good revision is:
changeset:   116117:d229008d60ac
user:        Sean Stangl
date:        Wed Dec 12 17:23:04 2012 -0800
summary:     Bug 808245, Part 4/6 - Compile RegExpShared at execution time. r=dvander

This iteration took 66.610 seconds to run.

Comment 5

[Josh Aas]

Marking fixed, resolved by bug 808245.

Comment 6

[Christian Holler (:decoder)]

I'm pretty sure that there is a possibility this is not fixed. Bug 808245 likely added new code paths such that the test in comment 0 is now taking these instead of the faulty ones. I'd like to have Sean confirm that this is really fixing it.

Comment 7

[Christian Holler (:decoder)]

Sean, could you comment on comment 6? Thanks! :)

Comment 8

[Sean Stangl [:sstangl]]

(In reply to Christian Holler (:decoder) from comment #7)
> Sean, could you comment on comment 6? Thanks! :)

Bug 808245 changed regexps to be compiled only when executed. The assertion still trips with the following modified testcase:

testRegexp("$.+$", "gi");
testRegexp(".*(?:)bc$", "gi");
testRegexp(".*(?:)bc$$.+$", "gi");
function testRegexp(res, mode) {
    re = new RegExp(res, mode);
    re.exec("foo");
}
var re2 = new RegExp(re);
re2.exec("foo");

Comment 9

[Christian Holler (:decoder)]

Thanks Sean, now tracking modified testcase.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test

Comment 11

[Christian Holler (:decoder)]

Yea, stupid me.

Comment 12

[Sean Stangl [:sstangl]]

(In reply to Christian Holler (:decoder) from comment #11)
> Yea, stupid me.

It might be worthwhile teaching the fuzzer to now execute most regexps that are built, since the act of merely creating a regexp no longer exercises any interesting codepaths.

Comment 13

[Christian Holler (:decoder)]

(In reply to Sean Stangl from comment #12)
> It might be worthwhile teaching the fuzzer to now execute most regexps that
> are built, since the act of merely creating a regexp no longer exercises any
> interesting codepaths.

Thanks, the fuzzer does that already. The reason why it wasn't in the test of comment 0 is that executions got removed during reduction. But the fuzzer does various stuff like test/exec with the regexp it generates :)

Comment 14

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 22bb671d4982).

Comment 16

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

dvander suggests sending this over to Naveed to find an owner.

Comment 17

[Andrew McCreight [:mccr8]]

We just passed the 6 month anniversary of this sec-high bug.  Is there somebody who can fix this?

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

One year birthday approaching. Does this still affect trunk?

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Is this still an issue? It is our third oldest open sec-high at this point.

Comment 20

[Sean Stangl [:sstangl]]

This is still an issue, but Apple is no longer working on YARR, and the Mozilla JS team plans to remove it and switch to Google's irregexp, discussed in the linked js-engine-internals thread.

https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/GYKRKntBgiI

Comment 21

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

> This is still an issue, but Apple is no longer working on YARR, and the
> Mozilla JS team plans to remove it and switch to Google's irregexp,
> discussed in the linked js-engine-internals thread.

Here at the JS work week, we decided to monkey-patch YARR for now, until the day we decide that moving to irregexp is worthwhile.

Comment 22

[Sean Stangl [:sstangl]]

The testcase in Comment 8 no longer reproduces on Linux x86 or x86_64.

Comment 23

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

I did a quick retest on Mac 64-bit, and couldn't reproduce either. Decoder, would you like to try and reproduce again?

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Christian, is jsbugmon on PTO? :)

Comment 25

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 5b6e82e7bbbf).
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/fa67c55da0d5
user:        Sean Stangl
date:        Wed Oct 02 14:17:34 2013 -0700
summary:     Bug 916511 - Prevent underflow in YARR. r=nbp

This iteration took 1.066 seconds to run.

Comment 26

[Christian Holler (:decoder)]

Sean, did the fix in comment 25 fix this issue?

Comment 27

[Andrew McCreight [:mccr8]]

Marking WFM per comment 25.  Please reopen if needed.