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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: 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.

Comment 1

[Christian Holler (:decoder)]

Missed the revision, this is m-c c60535115ea1.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Wow, nice find, Christian. Looking into it now.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Note: 64b only.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Only 11 should be affected, because the RegExpPrivateCache landed there. Bug 634654.

Comment 5

[Johnny Stenback (:jst)]

Thanks Chris, flags updated.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Lookup cache again across possible GC.

The cache can totally disappear across the uncached RegExpPrivate creation.

Comment 7

[Jeff Walden [:Waldo]]

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!

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f50deaec86e7

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/mozilla-central/rev/f50deaec86e7

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 11

[Johnny Stenback (:jst)]

Thanks for the correction, Chris!

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

[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.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

(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!

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> 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.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

(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!

Comment 17

[u279076]

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

Comment 18

[Christian Holler (:decoder)]

(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.

Comment 19

[u279076]

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.

Comment 20

[Christian Holler (:decoder)]

(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.

Comment 21

[u279076]

Same error with changeset from comment 1.

Comment 22

[Christian Holler (:decoder)]

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.

Comment 23

[u279076]

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.

Comment 24

[Christian Holler (:decoder)]

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.