Closed Bug 726595 Opened 11 years ago Closed 11 years ago

RegExp related memory corruption (SIGTRAP + Valgrind Errors)


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox10 - unaffected
firefox11 - unaffected
firefox12 + unaffected
firefox13 + fixed
firefox-esr10 - unaffected


(Reporter: decoder, Assigned: luke)


(Keywords: crash, testcase, valgrind, Whiteboard: [sg:critical] js-triage-needed [advisory-tracking+])

Crash Data


(3 files)

The attached test crashes on mozilla-central revision 4a9a6ffd1f21 (options -m -n -a). The testcase is very fragile, I wasn't able to condense it into a single file even with evaluate.

Valgrind traces:

==14035== Invalid read of size 4
==14035==    at 0x81E4B2E: js::RegExpShared::Guard::init(js::RegExpShared&) (RegExpObject.h:328)
==14035==    by 0x81E6431: RegExpGuard::init(JSContext*, js::CallArgs, bool) (jsstr.cpp:1430)
==14035==    by 0x81DE23C: js::str_replace(JSContext*, unsigned int, JS::Value*) (jsstr.cpp:2238)
==14035==    by 0x81489E7: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:311)
==14035==  Address 0x911facc is 28 bytes inside a block of size 32 free'd
==14035==    at 0x48D0C02: free (vg_replace_malloc.c:366)
==14035==    by 0x82ADB7F: js_free (Utility.h:152)
==14035==    by 0x82B21A8: void js::Foreground::delete_<js::RegExpShared>(js::RegExpShared*) (in /srv/repos/mozilla-central/js/src/debug32/shell/js)
==14035==    by 0x82AF7FE: js::RegExpCompartment::purge() (RegExpObject.cpp:624)

[ .. More invalid reads of same kind at:
   js::RegExpShared::global() const (RegExpObject.h:356)
   js::RegExpShared::pairCount() const (RegExpObject.h:352)
   js::RegExpShared::sticky() const (RegExpObject.h:358)
   JSC::Yarr::YarrCodeBlock::isFallBack() (YarrJIT.h:67)

==14035== Process terminating with default action of signal 5 (SIGTRAP)
==14035==    at 0x7050001: ???
==14035==    by 0x83848EE: JSC::Yarr::execute(JSC::Yarr::YarrCodeBlock&, unsigned short const*, unsigned int, unsigned int, int*) (YarrJIT.cpp:2468)
==14035==    by 0x82AEC3A: js::detail::RegExpCode::execute(JSContext*, unsigned short const*, unsigned int, unsigned int, int*, unsigned int) (RegExpObject.cpp:309)
==14035==    by 0x82AF659: js::RegExpShared::execute(JSContext*, unsigned short const*, unsigned int, unsigned int*, js::MatchPairs**) (RegExpObject.cpp:575)

S-s, assuming sg:critical (memory corruption).
Unfortunately I cannot reproduce locally, but the pasted stacks gives me a pretty good idea of what the problem is: naked RegExpShared* are being passed around which leaves a window for GC to occur before they are protected.  This patch uses the static type system to force correct RegExpGuard usage.

Decoder, can you verify whether this patch (applies on m-i tip f07a6060cc5a) fixes the problem?  Thanks!
Assignee: general → luke
Attachment #598984 - Flags: feedback?(choller)
(In reply to Luke Wagner [:luke] from comment #1)

> Decoder, can you verify whether this patch (applies on m-i tip f07a6060cc5a)
> fixes the problem?  Thanks!

Unfortunately I cannot even verify the issue on that m-i revision without the patch (I don't see anything in Valgrind either, it might just be very fragile). I do see the issue on the original m-c revision and I tried applying the patch there but some hunks fail and I tried to manually resolve that but the code looks entirely different at some point.

Any idea how to proceed?
Ah, yes, incremental gc landed in the interim :)  I backported the patch to apply to the cset in comment 0 (4a9a6ffd1f21).  Perhaps now you can verify?
Attachment #599215 - Flags: feedback?(choller)
Comment on attachment 599215 [details] [diff] [review]
patch backported to apply to 4a9a6ffd1f21

Confirmed that this fixes the crash and the invalid reads. The uninitialized value warnings are still there, but maybe that's an unrelated issue:

==7270== Conditional jump or move depends on uninitialised value(s)
==7270==    at 0x80B9F48: JSRuntime::createExecutableAllocator(JSContext*) (jscntxt.cpp:143)
==7270==    by 0x82B2895: JSRuntime::getExecutableAllocator(JSContext*) (jscntxt.h:224)
==7270==    by 0x82B0F08: js::detail::RegExpCode::compile(JSContext*, JSLinearString&, unsigned int*, js::RegExpFlag) (RegExpObject.cpp:263)
==7270==    by 0x82B18EA: js::RegExpShared::compile(JSContext*, JSAtom*) (RegExpObject.cpp:524)
==7270==    by 0x82B37DC: js::RegExpCompartment::get(JSContext*, JSAtom*, JSAtom*, js::RegExpFlag, js::RegExpCompartment::Type, js::RegExpGuard*) (RegExpObject.cpp:646)
==7270==    by 0x82B1D75: js::RegExpCompartment::get(JSContext*, JSAtom*, js::RegExpFlag, js::RegExpGuard*) (RegExpObject.cpp:675)
==7270==    by 0x82B12DD: js::RegExpObject::createShared(JSContext*, js::RegExpGuard*) (RegExpObject.cpp:401)
==7270==    by 0x81E7C71: js::RegExpObject::getShared(JSContext*, js::RegExpGuard*) (RegExpObject-inl.h:82)
==7270==    by 0x82B0804: js::RegExpObjectBuilder::clone(js::RegExpObject*, js::RegExpObject*) (RegExpObject.cpp:139)
==7270==    by 0x82B1F6E: js::CloneRegExpObject(JSContext*, JSObject*, JSObject*) (RegExpObject.cpp:714)
==7270==    by 0x813FB3E: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2877)
==7270==    by 0x82BDEC0: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1080)

==7270== Conditional jump or move depends on uninitialised value(s)
==7270==    at 0x80BD062: JSC::ExecutableAllocator::setRandomize(bool) (ExecutableAllocator.h:254)
==7270==    by 0x80BC39E: JSContext::updateJITEnabled() (jscntxt.cpp:1257)
==7270==    by 0x80BBEFC: JSContext::resetCompartment() (jscntxt.cpp:1061)
==7270==    by 0x825554F: js::ContextStack::popFrame(js::FrameGuard const&) (Stack.cpp:869)
==7270==    by 0x8149B44: js::FrameGuard::~FrameGuard() (in /srv/repos/mozilla-central/js/src/debug32-patched/shell/js)
==7270==    by 0x814DB90: js::ExecuteFrameGuard::~ExecuteFrameGuard() (Stack.h:1774)
==7270==    by 0x813385F: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:640)
==7270==    by 0x8133A15: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:698)
==7270==    by 0x8084839: JS_ExecuteScript (jsapi.cpp:5300)
==7270==    by 0x804C314: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:477)
==7270==    by 0x8057BCC: ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) (js.cpp:5163)
==7270==    by 0x8057E08: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:5246)
Attachment #599215 - Flags: feedback?(choller) → feedback+
Sweet, thanks.  With respect to those warnings, I was able to see those locally (they don't seem fragile); perhaps file a new bug and cc me and Chris?  This looks like the jit hardening stuff.
Attachment #598984 - Flags: review?(christopher.leary)
Attachment #598984 - Flags: feedback?(choller)
Attachment #598984 - Flags: feedback+
Attachment #598984 - Flags: feedback+
Green on try
Comment on attachment 598984 [details] [diff] [review]
require RegExpGuard use everywhere

Review of attachment 598984 [details] [diff] [review]:

Like Luke and I talked about this is great because it's basically a little "handle" implementation for rooting regular expression guts.
Attachment #598984 - Flags: review?(christopher.leary) → review+
Guessing that 12 is affected.
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-needed [advisory-tracking+]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.