1566784 - stack-buffer-overflow in mozilla::dom::DOMMatrixReadOnly::Stringify

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Nils]

Attachment: crash.html (minimised testcase)

The following testcase crashes the latest ASAN build of Firefox 70.0a1 (SourceStamp=29e9dde37bd231a94959394554154ede52670c65).

crash.html:
<script>
o1=new DOMMatrix([0,1,2,3,4,5]);
o184=o1.rotateFromVector(-1,234881024);
o190=o184.scale3d(129);
o190.toString();
</script>

ASAN output:

==20415==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd3f693b99 at pc 0x7f6ced2cb16b bp 0x7ffd3f693730 sp 0x7ffd3f693728
WRITE of size 1 at 0x7ffd3f693b99 thread T0 (file:// Content)
#0 0x7f6ced2cb16a in Finalize /builds/worker/workspace/build/src/obj-firefox/dist/include/double-conversion/utils.h:276:24
#1 0x7f6ced2cb16a in JS::NumberToString(double, char (&) [25]) /builds/worker/workspace/build/src/js/src/jsnum.cpp:1490
#2 0x7f6ce159f0b1 in operator() /builds/worker/workspace/build/src/dom/base/DOMMatrix.cpp:512:5
#3 0x7f6ce159f0b1 in mozilla::dom::DOMMatrixReadOnly::Stringify(nsTSubstring<char16_t>&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/DOMMatrix.cpp:525
#4 0x7f6ce40de575 in mozilla::dom::DOMMatrixReadOnly_Binding::__stringifier(JSContext*, JS::Handle<JSObject*>, mozilla::dom::DOMMatrixReadOnly*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/DOMMatrixBinding.cpp:5851:24
#5 0x7f6ce4bf3e28 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3181:13
#6 0x7f6cec5eb8c7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:448:13
#7 0x7f6cec5eb8c7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:540
#8 0x7f6cec5cc296 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:599:10
#9 0x7f6cec5cc296 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3089
#10 0x7f6cec5b5caf in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:425:10
#11 0x7f6cec5f218f in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:787:13
#12 0x7f6cec5f295f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:820:10
#13 0x7f6cec8a787a in ExecuteScript(JSContext*, JS::Handle<JS::StackGCVector<JSObject*, js::TempAllocPolicy> >, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/CompilationAndEvaluation.cpp:468:10
#14 0x7f6ce1b0eb30 in nsJSUtils::ExecutionContext::ExecScript() /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:416:8
#15 0x7f6ce777914b in mozilla::dom::ExecuteCompiledScript(JSContext*, mozilla::dom::ScriptLoadRequest*, nsJSUtils::ExecutionContext&) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2588:16
#16 0x7f6ce7775a9f in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2812:20
#17 0x7f6ce776c677 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2315:10
#18 0x7f6ce7767bc3 in mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1862:10
#19 0x7f6ce7736f38 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1585:10
#20 0x7f6ce7735f3c in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:118:18
#21 0x7f6ce00518da in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:224:18
#22 0x7f6ce00518da in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:726
#23 0x7f6ce004b173 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:529:7
#24 0x7f6ce00582d0 in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:132:18
#25 0x7f6cdd11edc5 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:295:32
#26 0x7f6cdd15fd0c in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1225:14
#27 0x7f6cdd167b94 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#28 0x7f6cde58accf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
#29 0x7f6cde45285e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#30 0x7f6cde45285e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#31 0x7f6cde45285e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#32 0x7f6ce7c171d3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#33 0x7f6cec30cb6e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:919:20
#34 0x7f6cde45285e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#35 0x7f6cde45285e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#36 0x7f6cde45285e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#37 0x7f6cec30b6b1 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:754:34
#38 0x55b42c483173 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#39 0x55b42c483173 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:267
#40 0x7f6d02348b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
#41 0x55b42c3a46ac in _start (/home/nils/browser/firefox/firefox/firefox+0x456ac)

Address 0x7ffd3f693b99 is located in stack of thread T0 (file:// Content) at offset 761 in frame
#0 0x7f6ce159e26f in mozilla::dom::DOMMatrixReadOnly::Stringify(nsTSubstring<char16_t>&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/DOMMatrix.cpp:500

This frame has 24 object(s):
[32, 48) 'ref.tmp.i447' (line 506)
[64, 80) 'ref.tmp.i426' (line 506)
[96, 112) 'ref.tmp.i405' (line 506)
[128, 144) 'ref.tmp.i384' (line 506)
[160, 176) 'ref.tmp.i363' (line 506)
[192, 208) 'ref.tmp.i342' (line 506)
[224, 240) 'ref.tmp.i321' (line 506)
[256, 272) 'ref.tmp.i305' (line 506)
[288, 304) 'ref.tmp.i289' (line 506)
[320, 336) 'ref.tmp.i269' (line 506)
[352, 368) 'ref.tmp.i250' (line 506)
[384, 400) 'ref.tmp.i234' (line 506)
[416, 432) 'ref.tmp.i218' (line 506)
[448, 464) 'ref.tmp.i202' (line 506)
[480, 496) 'ref.tmp.i186' (line 506)
[512, 528) 'ref.tmp.i170' (line 506)
[544, 560) 'ref.tmp.i154' (line 506)
[576, 592) 'ref.tmp.i135' (line 506)
[608, 624) 'ref.tmp.i116' (line 506)
[640, 656) 'ref.tmp.i100' (line 506)
[672, 688) 'ref.tmp.i84' (line 506)
[704, 720) 'ref.tmp.i' (line 506)
[736, 761) 'cbuf' (line 501) <== Memory access at offset 761 overflows this variable
[800, 952) 'matrixStr' (line 502)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
(longjmp and C++ exceptions are supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/double-conversion/utils.h:276:24 in Finalize
Shadow bytes around the buggy address:
0x100027eca720: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
0x100027eca730: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
0x100027eca740: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
0x100027eca750: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
0x100027eca760: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
=>0x100027eca770: 00 00 00[01]f2 f2 f2 f2 00 00 00 00 00 00 00 00
0x100027eca780: 00 00 00 00 00 00 00 00 00 00 00 f3 f3 f3 f3 f3
0x100027eca790: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
0x100027eca7a0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
0x100027eca7b0: 00 f2 f2 f2 00 00 f2 f2 00 00 00 00 00 00 00 00
0x100027eca7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f3 f3
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==20415==ABORTING

Comment 1

[Nils]

Attachment: ASAN output

Comment 2

[Andrew McCreight [:mccr8]]

Possibly a regression from bug 1364875?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sure looks like it, but the issue may be in bug 1564589 instead. Lemme look at what's going on here.

Comment 4

[Jeff Walden [:Waldo]]

Yeah, I'd bet it's on the JS side of things given that stack. Most likely JS::MaximumNumberToStringLength is inadequately large, although how that can happen -- what numeric input will trigger it -- is entirely unobvious to me just looking at things. Presumably just running the testcase will give the relevant double value and then it's just a matter of bumping to account for that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

JS::MaximumNumberToStringLength is too small. In a debug build, using this testcase, I get:

Assertion failure: !is_finalized() && position_ + n < buffer_.length(), at mfbt/double-conversion/double-conversion/utils.h:258

At this point buffer_.length() is 25, as expected. position_ + n is also 25.

At this point position_ is 8. We are adding a 17-character string via StringBuilder::AddSubstring: "10984284297360395".

The actual number involved here is -1.0984284297360395e-06 or so. We land in DoubleToStringConverter::ToShortestIeeeNumber and it has this bit:

186       if ((decimal_in_shortest_low_ <= exponent) &&
187           (exponent < decimal_in_shortest_high_)) {
188         CreateDecimalRepresentation(decimal_rep, decimal_rep_length,

(it does CreateExponentialRepresentation if the exponent checks don't check out). In this case, exponent is 6, which is decimal_in_shortest_low_.

(-1.0984284297360395e-06).toString() gives "-0.0000010984284297360395" in JS, which matches the "10984284297360395" string that is being passed. Note that this string has length 25, which is exactly the failure mode we are seeing. I think the issue is that in this case we have those 6 leading 0, and that's longer than the max e+NNN that the determination of MaximumNumberToStringLength allowed for the exponent...

I think just bumping MaximumNumberToStringLength will do the trick, but it's worth carefully thinking about this.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Bug 1566784 - Increase the size of the buffer that JS::NumberToString fills with the result of ToString applied to the provided double to fix overflows. r=arai

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Bug 1566784 - Increase the size of the buffer that JS::NumberToString fills with the result of ToString applied to the provided double to fix overflows. r=arai

Depends on D38442

Comment 8

[Jeff Walden [:Waldo]]

The spec ToString algorithm is extraordinarily unclear what the maximum length of the string it returns can be, because it specifies its behavior as "find values that satisfy these constraints, then plug them into the appropriate set of steps". I spent far too long trying -- and failing -- to figure out a principled answer. :-(

It looks fairly likely that 25 is at least very close to the upper string limit, so let's just bump the buffer size to the next highest multiple-of-pointer-size -- a bump of a full seven characters! -- and assume that's way more than enough to be safe.

Comment 9

[Jeff Walden [:Waldo]]

This was only ever nightly-only and new, landed in inbound as both patches folded together:

https://hg.mozilla.org/integration/mozilla-inbound/rev/088932bf5d085c4560955d8047db28d08e0b7341

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/088932bf5d08

Comment 11

[Cristian Craciun]

Build ID 20190902191027
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:70.0) Gecko/20100101 Firefox/70.0

Verified as fixed on the latest Beta build (70.0b3) using macOS.