751412 - Invalid stack memory access in double_conversion::StringBuilder::AddSubstring

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Christian Holler (:decoder)]

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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bfc7e887ada0/try-linux64-debug/jsshell-linux-x86_64.zip

Comment 1

[Jeff Walden [:Waldo]]

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

Comment 2

[Christian Holler (:decoder)]

This bug is very easy to trigger so it would good if it could be fixed quickly :)

Comment 3

[Christian Holler (:decoder)]

Attachment: 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[4]. 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 4

[Nicholas Nethercote [inactive]]

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 5

[Christian Holler (:decoder)]

Comment on attachment 620889 [details] [diff] [review]
Patch

Also ran this through the JS tests and no patch-specific failures here. Flagging for checkin :)

Comment 6

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/84c861273421

Comment 7

[Jeff Walden [:Waldo]]

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?

Comment 8

[Christian Holler (:decoder)]

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

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/84c861273421