Closed Bug 1349531 Opened 3 years ago Closed 3 years ago
Sanitizer: global-buffer-overflow [@ __interceptor _getcwd] with WRITE of size 4098 or various Assertions/Crashes
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. Backtrace: ==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/sanitizer_common_interceptors.inc:2629 #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/sanitizer_common_interceptors.inc:2629 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 00f9 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/asan_interceptors.cc:239 #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.
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 Testcase: See attachment. Backtrace: 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.
NI for jandem as discussed on IRC.
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.
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
Status: NEW → ASSIGNED
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6994cbd66d97 Remove non-threadsafe static buffers, r=jandem
Seems worth taking next time we're pushing uplifts as a NPOTB ride-along to help reduce the fuzzing noise.
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')
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main54-]
You need to log in before you can comment on or make changes to this bug.