Closed
Bug 726595
Opened 12 years ago
Closed 12 years ago
RegExp related memory corruption (SIGTRAP + Valgrind Errors)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla13
Tracking | Status | |
---|---|---|
firefox10 | - | unaffected |
firefox11 | - | unaffected |
firefox12 | + | unaffected |
firefox13 | + | fixed |
firefox-esr10 | - | unaffected |
People
(Reporter: decoder, Assigned: luke)
Details
(Keywords: crash, testcase, valgrind, Whiteboard: [sg:critical] js-triage-needed [advisory-tracking+])
Crash Data
Attachments
(3 files)
1.75 KB,
application/x-compressed-tar
|
Details | |
42.44 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
42.70 KB,
patch
|
decoder
:
feedback+
|
Details | Diff | Splinter Review |
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) JSC::MacroAssemblerCodePtr::executableAddress() ] ==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).
Assignee | ||
Comment 1•12 years ago
|
||
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!
Reporter | ||
Comment 2•12 years ago
|
||
(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?
Assignee | ||
Comment 3•12 years ago
|
||
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)
Reporter | ||
Comment 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #598984 -
Flags: review?(christopher.leary)
Attachment #598984 -
Flags: feedback?(choller)
Attachment #598984 -
Flags: feedback+
Assignee | ||
Updated•12 years ago
|
Attachment #598984 -
Flags: feedback+
Assignee | ||
Comment 6•12 years ago
|
||
Green on try
Comment 7•12 years ago
|
||
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+
Comment 8•12 years ago
|
||
Guessing that 12 is affected.
status-firefox10:
--- → wontfix
status-firefox11:
--- → wontfix
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox10:
--- → -
tracking-firefox11:
--- → -
tracking-firefox12:
--- → +
tracking-firefox13:
--- → +
Assignee | ||
Comment 9•12 years ago
|
||
Nope, this is only on nightly.
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bfdb22d2d4
Target Milestone: --- → mozilla13
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/24bfdb22d2d4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
tracking-firefox-esr10:
--- → -
Updated•12 years ago
|
Group: core-security
Updated•12 years ago
|
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-needed [advisory-tracking+]
Reporter | ||
Updated•11 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•