Closed Bug 1349531 Opened 3 years ago Closed 3 years ago

AddressSanitizer: global-buffer-overflow [@ __interceptor_getcwd] with WRITE of size 4098 or various Assertions/Crashes


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox-esr52 --- fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed


(Reporter: decoder, Assigned: sfink)


(Blocks 1 open bug)


(6 keywords, Whiteboard: [jsbugmon:][adv-main54-])


(3 files)

The following testcase crashes on mozilla-central revision e1576dd8bd9d (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --baseline-eager --ion-extra-checks --ion-offthread-compile=off --ion-aa=flow-sensitive):

See attachment.


==26154==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000041c2221 at pc 0x0000004a996d bp 0x7f34c1ddc230 sp 0x7f34c1ddb9e0
WRITE of size 4098 at 0x0000041c2221 thread T11
    #0 0x4a996c in __interceptor_getcwd /srv/repos/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/
    #1 0x54bcc7 in js::shell::ResolvePath(JSContext*, JS::Handle<JSString*>, js::shell::PathResolutionMode) js/src/shell/OSObject.cpp:144:27
    #2 0x58df1d in LoadScript(JSContext*, unsigned int, JS::Value*, bool) js/src/shell/js.cpp:1377:15
    #3 0x7505a8 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:282:15
    #4 0x7505a8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:448
    #5 0x96a555 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:2347:14
    #6 0x2e094b68d2d8  (<unknown module>)

0x0000041c2221 is located 0 bytes to the right of global variable 'buffer' defined in 'js/src/shell/OSObject.cpp:129:17' (0x41c1220) of size 4097
SUMMARY: AddressSanitizer: global-buffer-overflow /srv/repos/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/ in __interceptor_getcwd
Shadow bytes around the buggy address:
  0x000080830430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x000080830440: 00 00 00 00[01]f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080830450: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Global redzone:          f9

Thread T11 created by T0 here:
    #0 0x48062d in __interceptor_pthread_create /srv/repos/llvm/projects/compiler-rt/lib/asan/
    #1 0x71948c in js::Thread::create(void* (*)(void*), void*) js/src/threading/posix/Thread.cpp:104:7
    #2 0x5a7973 in bool js::Thread::init<void (&)(void*), WorkerInput*&>(void (&)(void*), WorkerInput*&) js/src/threading/Thread.h:117:12
    #3 0x591440 in EvalInThread(JSContext*, unsigned int, JS::Value*, bool) js/src/shell/js.cpp:3702:21
    #4 0x2e094b633783  (<unknown module>)

This bug is particularly interesting because it crashes in so many ways and behaves non-deterministically. I tried to debug this and it appears to me that we first drop out of JIT and then call load, but the data with which we call load is already clobbered in some way. I suspect this is a potential memory corruption that is hard to detect. The attached testcase has a chance of about 3-4% to reproduce on either an ASan build or a debug build. I will also try to attach a larger testcase that is more reliable. I assume this is likely s-s and sec-critical, the fact that we crash in load() here doesn't seem to be crucial for the bug.
Attached file Testcase
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
This is an automated crash issue comment:

Summary: Assertion failure: gHelperThreadState, at js/src/vm/HelperThreads.h:305
Build version: mozilla-central revision e1576dd8bd9d
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --thread-count=2 --disable-oom-functions --baseline-eager --ion-extra-checks --ion-offthread-compile=off --ion-aa=flow-sensitive


See attachment.


 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe89ff700 (LWP 26588)]
__strncpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcpy-sse2-unaligned.S:1671
#0  __strncpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcpy-sse2-unaligned.S:1671
#1  0x0000000000448d72 in strncpy (__len=<optimized out>, __src=<optimized out>, __dest=<optimized out>) at /usr/include/x86_64-linux-gnu/bits/string3.h:126
#2  js::shell::ResolvePath (cx=cx@entry=0x7ffff51f6000, filenameStr=..., filenameStr@entry=..., resolveMode=<optimized out>, resolveMode@entry=js::shell::RootRelative) at js/src/shell/OSObject.cpp:151
#3  0x0000000000451154 in LoadScript (cx=0x7ffff51f6000, argc=<optimized out>, vp=<optimized out>, scriptRelative=false) at js/src/shell/js.cpp:1377
#4  0x0000000000543660 in js::CallJSNative (cx=cx@entry=0x7ffff51f6000, native=0x451460 <Load(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:282
#5  0x0000000000538e3a in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff51f6000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:448
#6  0x0000000000539246 in InternalCall (cx=cx@entry=0x7ffff51f6000, args=...) at js/src/vm/Interpreter.cpp:493
#7  0x000000000053936a in js::CallFromStack (cx=cx@entry=0x7ffff51f6000, args=...) at js/src/vm/Interpreter.cpp:499
#8  0x000000000060e009 in js::jit::DoCallFallback (cx=0x7ffff51f6000, frame=0x7fffe89fe478, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffe89fe420, res=...) at js/src/jit/BaselineIC.cpp:2347
#9  0x00001c7dbfc192e4 in ?? ()
#43 0x0000000000000000 in ?? ()
rax	0x1e7c4c2	31966402
rbx	0x7fffe89fdd50	140737096179024
rcx	0xa0a0a0a0a3b2928	723401728383985960
rdx	0x0	0
rsi	0xb	11
rdi	0x1e86fe0	32010208
rbp	0x7fffe89fdda0	140737096179104
rsp	0x7fffe89fdd38	140737096179000
r8	0xffffffffffff54a1	-43871
r9	0x7fffec91ded7	140737162370775
r10	0x7fffed4c3040	140737174581312
r11	0x7ffff6cb95d0	140737333925328
r12	0x7ffff51f6000	140737305862144
r13	0x7fffe89fdd60	140737096179040
r14	0x0	0
r15	0x7fffe89fde30	140737096179248
rip	0x7ffff6bd8435 <__strncpy_sse2_unaligned+3685>
=> 0x7ffff6bd8435 <__strncpy_sse2_unaligned+3685>:	movdqa %xmm0,0x20(%rdi)
   0x7ffff6bd843a <__strncpy_sse2_unaligned+3690>:	movdqa %xmm0,0x30(%rdi)

This is the full testcase, it is a little more reliable than the small one, but still not 100% reproducing.
Attached file Testcase for comment 3
NI for jandem as discussed on IRC.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
I looked at this yesterday. The problem is the buffer we have in ResolvePath:

    static char buffer[PATH_MAX+1];

Multiple shell (worker) threads can access this buffer. Then, due to the strncpy call, it's possible for one of the threads to call strlen(buffer) and get a value that's PATH_MAX+1. As a result, the |sizeof(buffer) - (len+1)| argument to strncpy underflows and we corrupt memory and get all kinds of weird assertion failures.

Steve, should we just remove the "static" there? I'm not too worried about stack overflows in shell-only code.
Flags: needinfo?(jdemooij) → needinfo?(sphink)
It sounds like this is a bug in shell only code, so I'm marking it sec-other. Feel free to unhide it if you want.
Keywords: sec-other
I was re-filtering my secure bugmail and found this. Sorry for the delay.

Yes, you are absolutely correct. I have a bad habit of using static buffers for test code, which is stupid because the only thing it's doing here is saving stack space and introducing thread races.
Attachment #8871134 - Flags: review?(jdemooij)
Assignee: nobody → sphink
Group: javascript-core-security
Comment on attachment 8871134 [details] [diff] [review]
Remove non-threadsafe static buffers

Review of attachment 8871134 [details] [diff] [review]:

Makes sense.
Attachment #8871134 - Flags: review?(jdemooij) → review+
Pushed by
Remove non-threadsafe static buffers, r=jandem
Flags: needinfo?(sphink)
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Seems worth taking next time we're pushing uplifts as a NPOTB ride-along to help reduce the fuzzing noise.
Whiteboard: [jsbugmon:] → [jsbugmon:][checkin-needed-beta][checkin-needed-esr52]
Whiteboard: [jsbugmon:][checkin-needed-beta][checkin-needed-esr52] → [jsbugmon:][checkin-needed-esr52]
has problems with esr52

grafting 420951:a1827dca2447 "Bug 1349531 - Remove non-threadsafe static buffers, r=jandem a=NPOTB"
merging js/src/shell/OSObject.cpp
warning: conflicts while merging js/src/shell/OSObject.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(sphink)
Flags: needinfo?(sphink)
Whiteboard: [jsbugmon:][checkin-needed-esr52] → [jsbugmon:]
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main54-]
You need to log in before you can comment on or make changes to this bug.