Note: There are a few cases of duplicates in user autocompletion which are being worked on.

RegExp related GC Crash [@ JSRuntime::updateMallocCounter] (Memory corruption)

VERIFIED FIXED in Firefox 11

Status

()

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

People

(Reporter: decoder, Assigned: cdleary)

Tracking

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

Trunk
mozilla11
x86_64
Linux
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox8- unaffected, firefox9- unaffected, firefox10- unaffected, firefox11+ fixed, firefox-esr10 unaffected, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa-] js-triage-needed, crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 573522 [details]
shell testcase, unpack, chdir and read README

The attached test crashes on mozilla-central revision (options -m -n). This test was very hard to reduce and still requires the driver to run (see README in the tarball for running instructions). It also switched between error messages of all kind (asserts, crashes, glibc aborts... just the guru meditation was missing to make the set complete).


Backtrace from GDB:

Program received signal SIGSEGV, Segmentation fault.
0x000000000041a3ea in JSRuntime::updateMallocCounter (this=0xb25d10, nbytes=24) at ../../jscntxt.h:799
799             ptrdiff_t newCount = gcMallocBytes - ptrdiff_t(nbytes);
(gdb) bt
#0  0x000000000041a3ea in JSRuntime::updateMallocCounter (this=0xb25d10, nbytes=24) at ../../jscntxt.h:799
#1  0x000000000041a2fd in JSRuntime::malloc_ (this=0xb25d10, bytes=24, cx=0x0) at ../../jscntxt.h:735
#2  0x000000000046ecc7 in js::RuntimeAllocPolicy::malloc_ (this=0xb2e9f0, bytes=24) at /srv/repos/mozilla-central/js/src/jscntxt.h:2455
#3  0x00000000004701ae in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::createTable (alloc=..., capacity=1) at ./dist/include/js/HashTable.h:342
#4  0x0000000000596472 in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::changeTableSize (this=0xb2e9f0, deltaLog2=0) at ./dist/include/js/HashTable.h:556
#5  0x00000000006538ab in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=...) at ./dist/include/js/HashTable.h:712
#6  0x0000000000653527 in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=..., pentry=0x7fffffffb548) at ./dist/include/js/HashTable.h:735
#7  0x0000000000652ea5 in js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=..., k=@0x7fffffffb5e8, 
    v=@0x7fffffffb5f8) at ./dist/include/js/HashTable.h:1053
#8  0x0000000000652320 in js::RegExpPrivate::create (cx=0xb1eef0, source=0x7ffff6228340, flags=js::StickyFlag, ts=0x0) at ../vm/RegExpObject-inl.h:227
#9  0x000000000064ffaa in js::RegExpObject::makePrivate (this=0x7ffff6007340, cx=0xb1eef0) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:237
#10 0x000000000058ff86 in js::RegExpObject::getOrCreatePrivate (this=0x7ffff6007340, cx=0xb1eef0) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.h:161
#11 0x000000000064fcc8 in js::RegExpObjectBuilder::clone (this=0x7fffffffb750, other=0x7ffff6007340, proto=0x7ffff60071a8)
    at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:152
#12 0x0000000000650c0b in js_CloneRegExpObject (cx=0xb1eef0, obj=0x7ffff6007340, proto=0x7ffff60071a8) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:508
#13 0x00000000004f2bf6 in js::Interpret (cx=0xb1eef0, entryFrame=0x7ffff63fb140, interpMode=js::JSINTERP_NORMAL) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:4170

Valgrind message (does not crash in valgrind):

==14402== Invalid write of size 8
==14402==    at 0x652ECC: js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::add(js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::AddPtr&, JSAtom* const&, js::RegExpPrivate* const&) (HashTable.h:1056)
==14402==    by 0x65231F: js::RegExpPrivate::create(JSContext*, JSLinearString*, js::RegExpFlag, js::TokenStream*) (RegExpObject-inl.h:227)
==14402==    by 0x64FFA9: js::RegExpObject::makePrivate(JSContext*) (RegExpObject.cpp:237)
==14402==    by 0x58FF85: js::RegExpObject::getOrCreatePrivate(JSContext*) (RegExpObject.h:161)
==14402==    by 0x64FCC7: js::RegExpObjectBuilder::clone(js::RegExpObject*, js::RegExpObject*) (RegExpObject.cpp:152)
==14402==    by 0x650C0A: js_CloneRegExpObject(JSContext*, JSObject*, JSObject*) (RegExpObject.cpp:508)
==14402==    by 0x4F2BF5: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:4170)
==14402==  Address 0x5e5e548 is 328 bytes inside a block of size 768 free'd
==14402==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==14402==    by 0x40382B: js_free (Utility.h:183)
==14402==    by 0x4114F3: js::Foreground::free_(void*) (Utility.h:597)
==14402==    by 0x41A3D3: JSRuntime::free_(void*) (jscntxt.h:770)
==14402==    by 0x46ECEF: js::RuntimeAllocPolicy::free_(void*) (jscntxt.h:2457)
==14402==    by 0x4702D5: js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::destroyTable(js::RuntimeAllocPolicy&, js::detail::HashTableEntry<js::HashMapEntry<JSAtom*, js::RegExpPrivate*> >*, unsigned int) (HashTable.h:354)
==14402==    by 0x46FEBD: js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::~HashTable() (HashTable.h:415)
==14402==    by 0x46F50B: js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::~HashMap() (jsprvtd.h:200)
==14402==    by 0x46F530: void JSRuntime::delete_<js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy> >(js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>*) (in /srv/repos/mozilla-central/js/src/debug64/shell/js)
==14402==    by 0x46B731: js::ThreadData::purgeRegExpPrivateCache(JSRuntime*) (jscntxt.cpp:178)
==14402==    by 0x46B816: js_PurgeThreads_PostGlobalSweep(JSContext*) (jscntxt.cpp:358)
==14402==    by 0x4AAA23: GCCycle(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2919)

This could be another manifestation of bug 701332, but I am not sure on that. S-s and sg:critical because of invalid writes and obvious memory corruption.
(Reporter)

Comment 1

6 years ago
Missed the revision, this is m-c c60535115ea1.
Wow, nice find, Christian. Looking into it now.
Note: 64b only.

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → wontfix
status-firefox9: --- → affected
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox8: --- → -
tracking-firefox9: --- → +
Only 11 should be affected, because the RegExpPrivateCache landed there. Bug 634654.
Blocks: 634654
Thanks Chris, flags updated.
status-firefox10: affected → unaffected
status-firefox8: wontfix → unaffected
status-firefox9: affected → unaffected
tracking-firefox10: + → -
tracking-firefox9: + → -
Created attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

The cache can totally disappear across the uncached RegExpPrivate creation.
Assignee: general → cdleary
Status: NEW → ASSIGNED
Attachment #573642 - Flags: review?
Attachment #573642 - Flags: review? → review?(jwalden+bmo)
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

Review of attachment 573642 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/RegExpObject-inl.h
@@ +197,5 @@
> +    if (!cache)
> +        return false;
> +
> +    RegExpPrivateCache::Ptr p = cache->lookup(atom);
> +    if (p) {

if (RegExpPrivateCache::Ptr p = ...)

@@ +220,5 @@
> +    if (!cache)
> +        return false;
> +
> +    RegExpPrivateCache::AddPtr addPtr = cache->lookupForAdd(atom);
> +    if (addPtr) {

You could even do if (REPC::AP addPtr = ...) here, because addPtr is exposed to both arms of the if, but maybe that looks weird.  I personally am fine with it.

@@ +255,5 @@
>       * |RegExpPrivate|'s deallocation decref will first cause it to
>       * remove itself from the cache.
>       */
>  
> +    JSAtom *sourceAtom = &source->asAtom();

Keeping this as a reference would make for nicer interfaces, I think, with them defining the argument as non-null.

@@ +267,5 @@
>  
> +    /*
> +     * Note: allocations performed by this call may cause a garbage collection, so we have to
> +     * re-lookup the cache (and inside the cache) after the allocation is performed.
> +     */

This is nicely informative.  Yet if the interface as used here doesn't even expose the cache, isn't this comment in the wrong place, and shouldn't it be in cacheInsert or something?

@@ +417,5 @@
>  
> +#ifdef DEBUG
> +    this->~RegExpPrivate();
> +    memset(this, 0xcd, sizeof(*this));
> +    cx->free_(this);

Moar poisoningFree!
Attachment #573642 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50deaec86e7
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/f50deaec86e7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

Sorry jst, I was wrong about what made the merge. The RegExpPrivate cache is indeed in aurora, so 10 is affected.
Attachment #573642 - Flags: approval-mozilla-aurora?
Thanks for the correction, Chris!
status-firefox10: unaffected → affected
status-firefox11: affected → fixed
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

The RegExpPrivate cache is going to be backed out of Aurora (along with RegExpPrivate laziness), so the aurora flag is no longer necessary.
Attachment #573642 - Flags: approval-mozilla-aurora?
Whiteboard: [sg:critical] js-triage-needed → [sg:critical][qa+] js-triage-needed
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.
tracking-firefox-esr10: --- → 11+
(In reply to Lukas Blakk [:lsblakk] from comment #13)

Hi Lukas, this does not need to be tracked for ESR due to backout of the RegExpPrivateCache from that release. Thanks!
> this does not need to be tracked for ESR due to backout of the
> RegExpPrivateCache from that release. Thanks!

Fixing up flags to reflect that Fx 10 & ESR 10 are not affected, removing from tracking for ESR 10 as well as setting unaffected on appropriate flags.
status-firefox-esr10: --- → unaffected
status-firefox10: affected → unaffected
tracking-firefox-esr10: 11+ → ---
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #15)
> > this does not need to be tracked for ESR due to backout of the
> > RegExpPrivateCache from that release. Thanks!
> 
> Fixing up flags to reflect that Fx 10 & ESR 10 are not affected, removing
> from tracking for ESR 10 as well as setting unaffected on appropriate flags.

thank you Gary!
status1.9.2: --- → unaffected
Keywords: regression
Christian, could you give QA any assistance in verifying this fix? I'm currently seeing the following error when running your attached test in a debug shell built from latest-mozilla-beta:

ReferenceError: jsTestDriverEnd is not defined
Whiteboard: [sg:critical][qa+] js-triage-needed → [sg:critical][qa?] js-triage-needed
(Reporter)

Comment 18

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Christian, could you give QA any assistance in verifying this fix? I'm
> currently seeing the following error when running your attached test in a
> debug shell built from latest-mozilla-beta:
> 
> ReferenceError: jsTestDriverEnd is not defined

Did you manage to reproduce the issue in an older shell? The tests I create are not semantically correct most of the time, so error messages such as the one you got here are expected.
No, I wasn't able to reproduce in an early shell. I'm now pulling down the changeset you indicate in comment 1 to see if it reproduces there.
(Reporter)

Comment 20

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #19)
> No, I wasn't able to reproduce in an early shell. I'm now pulling down the
> changeset you indicate in comment 1 to see if it reproduces there.

Okay. If that does not work for you, just ping me on IRC, and I will try to verify this instead.
Same error with changeset from comment 1.
(Reporter)

Comment 22

5 years ago
I wasn't able to reproduce this issue either, even with the changeset specified by myself in comment 1. It's unlikely though that the issue is still present because the whole feature causing it was backed out and fuzzers haven't detected this anymore since then.
Given comment 22, QA won't be able to verify this fix. If someone is able to reproduce the original issue, please verify the fix in Firefox 11.
Whiteboard: [sg:critical][qa?] js-triage-needed → [sg:critical][qa-] js-triage-needed
(Reporter)

Comment 24

5 years ago
I'm marking this verified because we did everything possible to try and verify this but reproduction of the original issue wasn't successful as per comment 22.
Status: RESOLVED → VERIFIED
Group: core-security
(Reporter)

Updated

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