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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 3

[Christian Holler (:decoder)]

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.

Comment 4

[Christian Holler (:decoder)]

Attachment: Testcase for comment 3

Comment 5

[Christian Holler (:decoder)]

NI for jandem as discussed on IRC.

Comment 6

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment

Comment 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Remove non-threadsafe static buffers

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.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8871134 [details] [diff] [review]
Remove non-threadsafe static buffers

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

Makes sense.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6994cbd66d97
Remove non-threadsafe static buffers, r=jandem

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6994cbd66d97

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Seems worth taking next time we're pushing uplifts as a NPOTB ride-along to help reduce the fuzzing noise.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1827dca2447

Comment 15

[Carsten Book [:Tomcat]]

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')

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/70cd711c6ae87875525a3383fdd8dc696d62db78