The following test causes an invalid stack memory access on mozilla-central revision 89e9b9213670 (run with no options): parseInt(1.0e-7, 10); This test only triggered in debug builds for me, opt builds seem fine, so I assume this is not security-critical. Here's the backtrace: ==35473== ERROR: AddressSanitizer stack-buffer-overflow on address 0x7ffff220b3ab at pc 0xa0ee19 bp 0x7ffff220b300 sp 0x7ffff220b2e0 READ of size 1 at 0x7ffff220b3ab thread T0 #0 0xa0ee19 in strlen ??:0 #1 0x9fdef7 in double_conversion::StringBuilder::AddSubstring(char const*, int) /builds/slave/try-lnx64-dbg/build/mfbt/double-conversion/utils.h:216 #2 0x9fddb9 in double_conversion::DoubleToStringConverter::CreateExponentialRepresentation(char const*, int, int, double_conversion::StringBuilder*) const /builds/slave/try-lnx64-dbg/build/mfbt/double-conversion/double-conversion.cc:107 #3 0x9fe5ac in double_conversion::DoubleToStringConverter::ToShortestIeeeNumber(double, double_conversion::StringBuilder*, double_conversion::DoubleToStringConverter::DtoaMode) const /builds/slave/try-lnx64-dbg/build/mfbt/double-conversion/double-conversion.cc:191 #4 0x5de0cb in double_conversion::DoubleToStringConverter::ToShortest(double, double_conversion::StringBuilder*) const /builds/slave/try-lnx64-dbg/build/js/src/../../mfbt/double-conversion/double-conversion.h:159 #5 0x5db90c in js::FracNumberToCString(JSContext*, js::ToCStringBuf*, double, int) /builds/slave/try-lnx64-dbg/build/js/src/jsnum.cpp:1089 #6 0x5dbbc3 in js_NumberToStringWithBase(JSContext*, double, int) /builds/slave/try-lnx64-dbg/build/js/src/jsnum.cpp:1144 #7 0x6afebc in js::ToStringSlow(JSContext*, JS::Value const&) /builds/slave/try-lnx64-dbg/build/js/src/jsstr.cpp:3299 #8 0x5d8077 in js::num_parseInt(JSContext*, unsigned int, JS::Value*) /builds/slave/try-lnx64-dbg/build/js/src/jsnum.cpp:422 #9 0x5b694d in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) /builds/slave/try-lnx64-dbg/build/js/src/jscntxtinlines.h:314 #10 0x5b61f5 in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) /builds/slave/try-lnx64-dbg/build/js/src/jsinterp.cpp:512 #11 0x5ae1ea in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) /builds/slave/try-lnx64-dbg/build/js/src/jsinterp.cpp:2757 #12 0x590ea7 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) /builds/slave/try-lnx64-dbg/build/js/src/jsinterp.cpp:475 #13 0x5b7c6b in js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) /builds/slave/try-lnx64-dbg/build/js/src/jsinterp.cpp:674 #14 0x5b80d3 in js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) /builds/slave/try-lnx64-dbg/build/js/src/jsinterp.cpp:715 #15 0x481fd9 in JS_ExecuteScript /builds/slave/try-lnx64-dbg/build/js/src/jsapi.cpp:5258 #16 0x4262ac in Process(JSContext*, JSObject*, char const*, bool) /builds/slave/try-lnx64-dbg/build/js/src/shell/js.cpp:479 #17 0x423106 in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) /builds/slave/try-lnx64-dbg/build/js/src/shell/js.cpp:4697 #18 0x4228c2 in Shell(JSContext*, js::cli::OptionParser*, char**) /builds/slave/try-lnx64-dbg/build/js/src/shell/js.cpp:4779 #19 0x42390c in main /builds/slave/try-lnx64-dbg/build/js/src/shell/js.cpp:4988 #20 0x7f4d80b2430d in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258 Address 0x7ffff220b3ab is located at offset 43 in frame <double_conversion::DoubleToStringConverter::CreateExponentialRepresentation(char const*, int, int, double_conversion::StringBuilder*) const> of T0's stack: This frame has 1 object(s): [32, 37) 'buffer' HINT: this may be a false positive if your program uses some custom stack unwind mechanism (longjmp and C++ exceptions *are* supported) Shadow byte and word: 0x1ffffe441675: f4 0x1ffffe441670: f1 f1 f1 f1 05 f4 f4 f4 You can use the following jsshell build to verify/debug this issue: http://firstname.lastname@example.org/try-linux64-debug/jsshell-linux-x86_64.zip
Double-conversion code is in mfbt now. Not sure what I should do about the triage-needed on this, clearing it for now I guess...
This bug is very easy to trigger so it would good if it could be fixed quickly :)
Created attachment 620889 [details] [diff] [review] Patch Ok here's what I think is going on: The function DoubleToStringConverter::CreateExponentialRepresentation passes a stack variable "buffer" into the AddSubstring function of the StringBuilder, which is also part of the double-conversion package. This variable "buffer" is allocated here with a static size of 5: mxr.mozilla.org/mozilla-central/source/mfbt/double-conversion/double-conversion.cc#101 However, the buffer is not null-terminated, the first write goes to buffer. This wouldn't be a big problem if the AddSubstring function of the StringBuilder wouldn't have an ASSERT that uses strlen on the buffer passed in. AddressSanitizer detects that the null-byte accessed here is not part of the buffer and terminates the program. The attached patch makes buffer one larger and makes the last char a null-byte.
Comment on attachment 620889 [details] [diff] [review] Patch Review of attachment 620889 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty safe to me. If Asan says this fixes the problem, then I believe it.
Comment on attachment 620889 [details] [diff] [review] Patch Also ran this through the JS tests and no patch-specific failures here. Flagging for checkin :)
Er, wait a second -- this being imported code, the patch here should be added to mfbt/double-conversion/update.sh. And we should file a bug against upstream with the patch, too. Christian, can you get these things done?
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #7) > Er, wait a second -- this being imported code, the patch here should be > added to mfbt/double-conversion/update.sh. And we should file a bug against > upstream with the patch, too. Christian, can you get these things done? Filed follow-up bug 752059.