907118 - Crash in [@ JSRope::getCharsNonDestructiveInternal(js::ThreadSafeContext*, js::ScopedJSFreePtr<wchar_t>&, bool) ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[blinky]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130819030205

Steps to reproduce:

1- Go to http://dl.dropboxusercontent.com/u/95157096/85f61cf7/45gjzZVc.html
2- Go to "about:memory" then press "Measure".


Actual results:

Browser crash.

Crash Report: bp-f6efbca7-9666-4c43-aeec-d69c52130820

Comment 1

[blinky]

This happen since the version 26.0a1 (2013-08-06).

http://hg.mozilla.org/mozilla-central/rev/a0dd80f800e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130806030203

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

s-s per jlebar's request

Comment 3

[Justin Lebar (not reading bugmail)]

Here's the valgrind trace.

Looks like we're writing past the end of a malloc'ed string.

> Invalid write of size 2
>    at 0x7F35FD3: JSRope::getCharsNonDestructiveInternal(js::ThreadSafeContext*, js::ScopedJSFreePtr<unsigned short>&, bool) const (PodOperations.h:78)
>    by 0x7F35E87: JSRope::getCharsNonDestructive(js::ThreadSafeContext*, js::ScopedJSFreePtr<unsigned short>&) const (String.cpp:170)
>    by 0x7FF3D50: js::detail::HashTable<js::HashMapEntry<JSString*, JS::StringInfo>, js::HashMap<JSString*, JS::StringInfo, js::StringHashPolicy, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::prepareHash(JSString* const&) (String.h:1083)
>    by 0x7FF20B1: StatsCellCallback(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long) (HashTable.h:1373)
>    by 0x7EFFA91: js::IterateZonesCompartmentsArenasCells(JSRuntime*, void*, void (*)(JSRuntime*, void*, JS::Zone*), void (*)(JSRuntime*, void*, JSCompartment*), void (*)(JSRuntime*, void*, js::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) (Iteration.cpp:52)
>    by 0x7FF1A2F: JS::CollectRuntimeStats(JSRuntime*, JS::RuntimeStats*, JS::ObjectPrivateVisitor*) (jsmemorymetrics.cpp:473)
>    by 0x72BD826: xpc::JSMemoryMultiReporter::CollectReports(nsDataHashtable<nsUint64HashKey, nsCString>*, nsDataHashtable<nsUint64HashKey, nsCString>*, nsIMemoryMultiReporterCallback*, nsISupports*) (XPCJSRuntime.cpp:2576)
>    by 0x703C248: nsWindowMemoryReporter::CollectReports(nsIMemoryMultiReporterCallback*, nsISupports*) (nsWindowMemoryReporter.cpp:359)
>    by 0x799A1C5: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:162)
>    by 0x72D3752: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2808)
>    by 0x72D7EB9: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1315)
>    by 0x7EF850B: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:218)
> 
>  Address 0x1f273e32 is 0 bytes after a block of size 146 alloc'd
>    at 0x402D954: malloc (vg_replace_malloc.c:291)
>    by 0x7F35F04: JSRope::getCharsNonDestructiveInternal(js::ThreadSafeContext*, js::ScopedJSFreePtr<unsigned short>&, bool) const (Utility.h:141)
>    by 0x7F35E87: JSRope::getCharsNonDestructive(js::ThreadSafeContext*, js::ScopedJSFreePtr<unsigned short>&) const (String.cpp:170)
>    by 0x7FF3D50: js::detail::HashTable<js::HashMapEntry<JSString*, JS::StringInfo>, js::HashMap<JSString*, JS::StringInfo, js::StringHashPolicy, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::prepareHash(JSString* const&) (String.h:1083)
>    by 0x7FF20B1: StatsCellCallback(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long) (HashTable.h:1373)
>    by 0x7EFFA91: js::IterateZonesCompartmentsArenasCells(JSRuntime*, void*, void (*)(JSRuntime*, void*, JS::Zone*), void (*)(JSRuntime*, void*, JSCompartment*), void (*)(JSRuntime*, void*, js::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) (Iteration.cpp:52)
>    by 0x7FF1A2F: JS::CollectRuntimeStats(JSRuntime*, JS::RuntimeStats*, JS::ObjectPrivateVisitor*) (jsmemorymetrics.cpp:473)
>    by 0x72BD826: xpc::JSMemoryMultiReporter::CollectReports(nsDataHashtable<nsUint64HashKey, nsCString>*, nsDataHashtable<nsUint64HashKey, nsCString>*, nsIMemoryMultiReporterCallback*, nsISupports*) (XPCJSRuntime.cpp:2576)
>    by 0x703C248: nsWindowMemoryReporter::CollectReports(nsIMemoryMultiReporterCallback*, nsISupports*) (nsWindowMemoryReporter.cpp:359)
>    by 0x799A1C5: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:162)
>    by 0x72D3752: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2808)
>    by 0x72D7EB9: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1315)

Comment 4

[Justin Lebar (not reading bugmail)]

Given that it's complaining about PodOperations.h, it seems that it's the PodCopy here that's problematic:

> size_t len = node->length();
> PodCopy(pos, node->asLinear().chars(), len);
> pos += len;

Comment 5

[Justin Lebar (not reading bugmail)]

Yeah, this patch hits its MOZ_CRASH(), so somehow this rope has more chars than its length().

>diff --git a/js/src/vm/String.cpp b/js/src/vm/String.cpp
>index 141db69..23ab415 100644
>--- a/js/src/vm/String.cpp
>+++ b/js/src/vm/String.cpp
>@@ -181,16 +181,17 @@ JSRope::getCharsNonDestructiveInternal(ThreadSafeContext *cx, ScopedJSFreePtr<js
>                                        bool nullTerminate) const
> {
>     /*
>      * Perform non-destructive post-order traversal of the rope, splatting
>      * each node's characters into a contiguous buffer.
>      */
> 
>     size_t n = length();
>+    size_t observedLength = 0;
>     jschar *s;
>     if (cx)
>         s = cx->pod_malloc<jschar>(n + 1);
>     else
>         s = js_pod_malloc<jschar>(n + 1);
> 
>     if (!s)
>         return false;
>@@ -228,16 +229,21 @@ JSRope::getCharsNonDestructiveInternal(ThreadSafeContext *cx, ScopedJSFreePtr<js
>                  * stack. Non-leaf nodes in the rope contain no value to be
>                  * consumed.
>                  */
>                 nodeStack.popBack();
>             }
>         } else {
>             /* For leaf nodes, copy the characters into the buffer. */
>             size_t len = node->length();
>+            observedLength += len;
>+            if (observedLength >= n) {
>+                MOZ_CRASH();
>+            }
>+
>             PodCopy(pos, node->asLinear().chars(), len);
>             pos += len;
> 
>             nodeStack.popBack();
>         }
> 
>         prev = node;
>     }

Comment 6

[Jeff Walden [:Waldo]]

o/~ You do the hokey pokey and you hide the bug again o/~

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> o/~ You do the hokey pokey and you hide the bug again o/~

Yeah, what the heck?

Anyway, this looks like a bug in JS; I don't think I'm the right person to investigate this further.

Comment 8

[Andrew McCreight [:mccr8]]

Brian, could you take a look?  Shu is on PTO this week.

Comment 9

[Brian Hackett [Laid off!]]

If that crash is hitting then it seems like a bug in the string code or a misunderstood invariant about how ropes are structured.  I don't know the string code or what could cause that crash to trigger.

Comment 10

[Luke Wagner [:luke]]

Ropes are dags and this traversal assumes they are trees (using rope->leftChild() == prev to detect "am I coming from the left child" means an iloop when rope->rightChild() == rope->leftChild()).

I'll take the bug; this algorithm should be rewritten not to do any of this 'prev' funny business.  I'm also going to rename this function to something more befitting of the massive performance hazard it entails.

Comment 11

[Luke Wagner [:luke]]

and it leaks on OOM

Comment 12

[Luke Wagner [:luke]]

Attachment: fix

I'm pretty sure this isn't s-s since, afaics, this is only called from the memory reporter and when we are off the main thread and use ScopedThreadSafeStringInspector (which is only off-main-thread browser startup script parsing, which obviously isn't hitting this case, probably b/c it only operates on atoms, and PJS, which is not enabled).

Comment 13

[Justin Lebar (not reading bugmail)]

I can't speak to the other use, but there's no way for a page to trigger a memory report except by asking the user to open about:memory and click "measure", so I'm OK saying that's not s-s.

Comment 14

[Luke Wagner [:luke]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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