The default bug view has changed. See this FAQ.

RegExp related memory corruption (SIGTRAP + Valgrind Errors)

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: luke)

Tracking

(Blocks: 1 bug, {crash, testcase, valgrind})

Trunk
mozilla13
x86
Linux
crash, testcase, valgrind
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox10- unaffected, firefox11- unaffected, firefox12+ unaffected, firefox13+ fixed, firefox-esr10- unaffected)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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).
(Assignee)

Comment 1

5 years ago
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!
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #598984 - Flags: feedback?(choller)
(Reporter)

Comment 2

5 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

5 years ago
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?
Attachment #599215 - Flags: feedback?(choller)
(Reporter)

Comment 4

5 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

5 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

5 years ago
Attachment #598984 - Flags: review?(christopher.leary)
Attachment #598984 - Flags: feedback?(choller)
Attachment #598984 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #598984 - Flags: feedback+
(Assignee)

Comment 6

5 years ago
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.
status-firefox10: --- → wontfix
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox10: --- → -
tracking-firefox11: --- → -
tracking-firefox12: --- → +
tracking-firefox13: --- → +
(Assignee)

Comment 9

5 years ago
Nope, this is only on nightly.
status-firefox10: wontfix → unaffected
status-firefox11: wontfix → unaffected
status-firefox12: affected → unaffected
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bfdb22d2d4
Target Milestone: --- → mozilla13
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/24bfdb22d2d4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox13: affected → fixed
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED

Updated

5 years ago
status-firefox-esr10: --- → unaffected
tracking-firefox-esr10: --- → -
Group: core-security
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-needed [advisory-tracking+]
(Reporter)

Updated

4 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.