Last Comment Bug 751412 - Invalid stack memory access in double_conversion::StringBuilder::AddSubstring
: Invalid stack memory access in double_conversion::StringBuilder::AddSubstring
: testcase
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla15
Assigned To: Christian Holler (:decoder)
Depends on:
Blocks: 752059
  Show dependency treegraph
Reported: 2012-05-02 16:28 PDT by Christian Holler (:decoder)
Modified: 2012-05-05 03:33 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1016 bytes, patch)
2012-05-03 16:35 PDT, Christian Holler (:decoder)
n.nethercote: review+
gary: checkin+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-02 16:28:17 PDT
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/
    #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/
    #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:
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-02 16:49:28 PDT
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) 2012-05-03 14:51:10 PDT
This bug is very easy to trigger so it would good if it could be fixed quickly :)
Comment 3 Christian Holler (:decoder) 2012-05-03 16:35:12 PDT
Created attachment 620889 [details] [diff] [review]

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:

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 [:njn] 2012-05-03 23:10:50 PDT
Comment on attachment 620889 [details] [diff] [review]

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) 2012-05-04 07:30:16 PDT
Comment on attachment 620889 [details] [diff] [review]

Also ran this through the JS tests and no patch-specific failures here. Flagging for checkin :)
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2012-05-04 07:41:54 PDT
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-04 11:38:57 PDT
Er, wait a second -- this being imported code, the patch here should be added to mfbt/double-conversion/  And we should file a bug against upstream with the patch, too.  Christian, can you get these things done?
Comment 8 Christian Holler (:decoder) 2012-05-04 14:53:06 PDT
(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/  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 (⌚ UTC+1/+2) 2012-05-05 03:33:35 PDT

Note You need to log in before you can comment on or make changes to this bug.