Closed Bug 818604 Opened 12 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox18 --- unaffected
firefox19 --- wontfix
firefox20 --- wontfix
firefox28 --- wontfix
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: decoder, Assigned: sstangl)

References

Details

(6 keywords, Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,ignore])

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);
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.
No longer blocks: IonFuzz
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision c8a1314aa449).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
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.
Marking fixed, resolved by bug 808245.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sean, could you comment on comment 6? Thanks! :)
Flags: needinfo?(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");
Flags: needinfo?(sstangl)
Thanks Sean, now tracking modified testcase.
Whiteboard: [jsbugmon:] → [jsbugmon:update,testComment=8]
Whiteboard: [jsbugmon:update,testComment=8] → [jsbugmon:testComment=8]
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
Yea, stupid me.
Whiteboard: [jsbugmon:testComment=8] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747]
(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.
(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 :)
Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747] → [jsbugmon:testComment=8,origRev=6c23f41b0747]
JSBugMon: Cannot process bug: Unknown exception (check manually)
Whiteboard: [jsbugmon:testComment=8,origRev=6c23f41b0747] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747]
Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm]
Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 22bb671d4982).
dvander suggests sending this over to Naveed to find an owner.
Assignee: general → nihsanullah
Flags: needinfo?(nihsanullah)
We just passed the 6 month anniversary of this sec-high bug.  Is there somebody who can fix this?
One year birthday approaching. Does this still affect trunk?
Assignee: nihsanullah → sstangl
Flags: needinfo?(nihsanullah)
Group: javascript-core-security
Is this still an issue? It is our third oldest open sec-high at this point.
Flags: needinfo?(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
Flags: needinfo?(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.

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.
Flags: needinfo?(sstangl)
The testcase in Comment 8 no longer reproduces on Linux x86 or x86_64.
Flags: needinfo?(sstangl)
I did a quick retest on Mac 64-bit, and couldn't reproduce either. Decoder, would you like to try and reproduce again?
Flags: needinfo?(choller)
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,ignore] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,bisectfix]
Christian, is jsbugmon on PTO? :)
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,bisectfix] → [jsbugmon:update,testComment=8,origRev=6c23f41b0747,reconfirm,ignore]
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.
Sean, did the fix in comment 25 fix this issue?
Flags: needinfo?(choller) → needinfo?(sstangl)
Marking WFM per comment 25.  Please reopen if needed.
Status: REOPENED → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → WORKSFORME
Flags: needinfo?(sstangl)
Group: javascript-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.