Closed Bug 751412 Opened 9 years ago Closed 9 years ago

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


(Core :: MFBT, defect)

Not set





(Reporter: decoder, Assigned: decoder)



(Keywords: testcase, Whiteboard: [asan][asan-test-failure])


(1 file)

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:
Whiteboard: [asan][asan-test-failure] → [asan][asan-test-failure] js-triage-needed
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...
Assignee: general → nobody
Component: JavaScript Engine → MFBT
QA Contact: general → mfbt
Whiteboard: [asan][asan-test-failure] js-triage-needed → [asan][asan-test-failure]
This bug is very easy to trigger so it would good if it could be fixed quickly :)
Attached patch PatchSplinter 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.
Assignee: nobody → choller
Attachment #620889 - Flags: review?(n.nethercote)
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.
Attachment #620889 - Flags: review?(n.nethercote) → review+
Comment on attachment 620889 [details] [diff] [review]

Also ran this through the JS tests and no patch-specific failures here. Flagging for checkin :)
Attachment #620889 - Flags: checkin?(gary)
Attachment #620889 - Flags: checkin?(gary) → checkin+
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?
Blocks: 752059
(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.
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.