Last Comment Bug 726595 - RegExp related memory corruption (SIGTRAP + Valgrind Errors)
: RegExp related memory corruption (SIGTRAP + Valgrind Errors)
Status: VERIFIED FIXED
[sg:critical] js-triage-needed [advis...
: crash, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla13
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-02-13 07:42 PST by Christian Holler (:decoder)
Modified: 2013-03-11 08:41 PDT (History)
8 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
-
unaffected
+
unaffected
+
fixed
-
unaffected


Attachments
shell testcase (32 bit dbg), unpack, chdir and run main.js with options "-n -m -a" (1.75 KB, application/x-compressed-tar)
2012-02-13 07:42 PST, Christian Holler (:decoder)
no flags Details
require RegExpGuard use everywhere (42.44 KB, patch)
2012-02-20 15:12 PST, Luke Wagner [:luke]
cdleary: review+
Details | Diff | Splinter Review
patch backported to apply to 4a9a6ffd1f21 (42.70 KB, patch)
2012-02-21 09:14 PST, Luke Wagner [:luke]
choller: feedback+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-02-13 07:42:25 PST
Created attachment 596684 [details]
shell testcase (32 bit dbg), unpack, chdir and run main.js with options "-n -m -a"

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).
Comment 1 Luke Wagner [:luke] 2012-02-20 15:12:19 PST
Created attachment 598984 [details] [diff] [review]
require RegExpGuard use everywhere

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!
Comment 2 Christian Holler (:decoder) 2012-02-20 16:05:54 PST
(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?
Comment 3 Luke Wagner [:luke] 2012-02-21 09:14:32 PST
Created attachment 599215 [details] [diff] [review]
patch backported to apply to 4a9a6ffd1f21

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?
Comment 4 Christian Holler (:decoder) 2012-02-21 09:43:21 PST
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)
Comment 5 Luke Wagner [:luke] 2012-02-21 09:52:56 PST
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.
Comment 6 Luke Wagner [:luke] 2012-02-22 10:45:23 PST
Green on try
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2012-02-23 11:09:24 PST
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.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 13:36:26 PST
Guessing that 12 is affected.
Comment 9 Luke Wagner [:luke] 2012-02-23 13:48:45 PST
Nope, this is only on nightly.
Comment 11 Luke Wagner [:luke] 2012-02-24 08:14:53 PST
https://hg.mozilla.org/mozilla-central/rev/24bfdb22d2d4

Note You need to log in before you can comment on or make changes to this bug.