Created attachment 320681 [details] Test Load the attached test with debug ASAN WebKitTestRunner: Checked version: 35511bd OS: macOS Sierra (10.12.6) <audio controls="false" onstalled="this.getElementsByTagName('input').prototype.getElementsByTagName('td')"></audio> Backtrace: ================================================================= ==11989==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fff5da6ad00 at pc 0x0001021df9eb bp 0x7fff5da68ce0 sp 0x7fff5da684a0 READ of size 1024 at 0x7fff5da6ad00 thread T0 #0 0x1021df9ea in __asan_memcpy (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/8.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x419ea) #1 0x12ccae191 in JSC::Probe::Page::Page(void*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2e77191) #2 0x12ccae1bc in JSC::Probe::Page::Page(void*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2e771bc) #3 0x12ccaf3c8 in JSC::Probe::Stack::ensurePageFor(void*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2e783c8) #4 0x12b4293c7 in JSC::Probe::Stack::pageFor(void*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x15f23c7) #5 0x12b439e76 in void JSC::Probe::Stack::set<JSC::JSValue>(void*, JSC::JSValue) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1602e76) #6 0x12b439c94 in void JSC::Probe::Frame::set<JSC::JSValue>(long, JSC::JSValue) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1602c94) #7 0x12b42352b in void JSC::Probe::Frame::setOperand<JSC::JSValue>(int, JSC::JSValue) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x15ec52b) #8 0x12b41f895 in JSC::DFG::OSRExit::executeOSRExit(JSC::Probe::Context&) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x15e8895) #9 0x12ccac026 in JSC::Probe::executeProbe(JSC::Probe::State*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2e75026) #10 0x12c8558ea in ctiMasmProbeTrampoline (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2a1e8ea) #11 0x62f00000e3ff (<unknown module>) Address 0x7fff5da6ad00 is located in stack of thread T0 at offset 0 in frame #0 0x12c1fa55f in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x23c355f) This frame has 5 object(s): [32, 40) 'retval' <== Memory access at offset 0 partially underflows this variable [64, 112) 'scope' <== Memory access at offset 0 partially underflows this variable [144, 168) 'agg.tmp' <== Memory access at offset 0 partially underflows this variable [208, 216) 'coerce' <== Memory access at offset 0 partially underflows this variable [240, 248) 'result' <== Memory access at offset 0 partially underflows this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-underflow (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/8.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x419ea) in __asan_memcpy Shadow bytes around the buggy address: 0x1fffebb4d550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x1fffebb4d5a0:[f1]f1 f1 f1 00 f2 f2 f2 00 00 00 00 00 00 f2 f2 0x1fffebb4d5b0: f2 f2 00 00 00 f2 f2 f2 f2 f2 00 f2 f2 f2 00 f3 0x1fffebb4d5c0: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d5d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebb4d5e0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 0x1fffebb4d5f0: 00 f2 f2 f2 00 f2 f2 f2 00 00 00 00 00 00 f2 f2 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==11989==ABORTING #CRASHED - com.apple.WebKit.WebContent.Development (pid 11989) LEAK: 1 WebProcessPool LEAK: 1 WebPageProxy
<rdar://problem/34417206>
This is a false positive by ASan. ASan does not like the Probe::Page code reading beyond the stackPointer. Page is only constructed in Stack::ensurePageFor(), and Stack::ensurePageFor() already does a RELEASE_ASSERT that the requested address is within the stack bounds. Since Page::size is 1K, and the stack is allocated in multiples of 1Ks (and is guaranteed to be 1K aligned in practice, this is not an issue at all.
Created attachment 320712 [details] proposed patch.
Comment on attachment 320712 [details] proposed patch. r=me.
Created attachment 320715 [details] proposed patch. Saam pointed out that I also need to suppress ASan on Page::flushWrites() as well.
Comment on attachment 320715 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=320715&action=review r-, as this doesn't fix ASanified builds. > Source/JavaScriptCore/ChangeLog:16 > + I've also added the SUPPRESS_ASAN attribute to tell ASan to ignore this > + constructor function. Unfortunately, Clang does not seem to honor that attribute > + (see <rdar://problem/34422508>). Regardless, this fix is correct and should be > + landed. The attribute only applies to the function it's on, and the violation occurs in a function called by it. It is expected behavior that SUPPRESS_ASAN doesn't help here, and we shouldn't be adding it. We've seen this before, the solution was to avoid using memcpy on poisoned memory (see for example copyMemory in MachineStackMarker.cpp).
The DFG OSR Exit patch has been rolled out in r222009: <http://trac.webkit.org/r222009> due to https://bugs.webkit.org/show_bug.cgi?id=176888. I'll fix this when I re-land the DFG OSR patch. *** This bug has been marked as a duplicate of bug 175144 ***
Actually, this is a Probe implementation issue, and not a DFG OSR exit issue. I'll fix it right away.
<rdar://problem/34436415>
Created attachment 320822 [details] proposed patch.
Attachment 320822 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:40: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: vulnerab [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320822 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=320822&action=review > Source/JavaScriptCore/assembler/ProbeStack.h:77 > + size_t numberOfChunks = sizeof(T) / s_chunkSize; I think you want to round up not down here, especially when s_chunkSize doesn't evenly divide sizeof(T). Also, you could just static_assert(sizeof(T) % s_chunkSize == 0) if you want > Source/JavaScriptCore/assembler/ProbeStack.h:122 > + static constexpr size_t s_pageSize = is64Bit() ? 512 : 256; // 64 * sizeof(uintptr_t) Why not just write 64 * sizeof(uintptr_t) here instead of the comment? > Source/JavaScriptCore/assembler/ProbeStack.h:132 > + static constexpr size_t s_chunkSizeShift = is64Bit() ? 3 : 2; maybe add: static_assert(1 << chunkSizeShift == sizeof(uintptr_t), "")? > Source/JavaScriptCore/assembler/testmasm.cpp:603 > + stack.set<double>(p++, 1.234567); why is this needed? > Source/JavaScriptCore/assembler/testmasm.cpp:634 > + CHECK_EQ(stack.get<double>(p++), 1.234567); ditto > Source/WTF/wtf/StdLibExtras.h:209 > + return reinterpret_cast<T*>(roundUpToMultipleOf<divisor>(reinterpret_cast<size_t>(x))); Please add: static_assert(sizeof(size_t) == sizeof(uintptr_t) or please use uintptr_t
Comment on attachment 320822 [details] proposed patch. Will apply Saam's feedback.
Comment on attachment 320822 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=320822&action=review >> Source/JavaScriptCore/assembler/ProbeStack.h:77 >> + size_t numberOfChunks = sizeof(T) / s_chunkSize; > > I think you want to round up not down here, especially when s_chunkSize doesn't evenly divide sizeof(T). > Also, you could just static_assert(sizeof(T) % s_chunkSize == 0) if you want Fixed. I changed it to: roundUpToMultipleOf<sizeof(T)>(s_chunkSize) / s_chunkSize; >> Source/JavaScriptCore/assembler/ProbeStack.h:122 >> + static constexpr size_t s_pageSize = is64Bit() ? 512 : 256; // 64 * sizeof(uintptr_t) > > Why not just write 64 * sizeof(uintptr_t) here instead of the comment? Fixed. >> Source/JavaScriptCore/assembler/ProbeStack.h:132 >> + static constexpr size_t s_chunkSizeShift = is64Bit() ? 3 : 2; > > maybe add: > static_assert(1 << chunkSizeShift == sizeof(uintptr_t), "")? I already do "static_assert(s_chunkSize == (1 << s_chunkSizeShift), "bad chunkSizeShift");" below that covers this. >> Source/JavaScriptCore/assembler/testmasm.cpp:603 >> + stack.set<double>(p++, 1.234567); > > why is this needed? 32-bit x86 rounding. The verification on read back below fails because it reads 1.2345678 instead. >> Source/JavaScriptCore/assembler/testmasm.cpp:634 >> + CHECK_EQ(stack.get<double>(p++), 1.234567); > > ditto Answered above. >> Source/WTF/wtf/StdLibExtras.h:209 >> + return reinterpret_cast<T*>(roundUpToMultipleOf<divisor>(reinterpret_cast<size_t>(x))); > > Please add: > static_assert(sizeof(size_t) == sizeof(uintptr_t) > or > please use uintptr_t I think the 2 (size_t and uintptr_t) should be the same. I'm using size_t because the roundUpToMultipleOf() function it forwards to takes size_t. I'll add the static_assert anyway just to be paranoid. Actually, I'll assert that sizeof(T*) == sizeof(size_t), which is what we really need for this to be correct.
Created attachment 320832 [details] patch for landing.
Attachment 320832 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:40: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: vulnerab [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review. Landed in r222058: <http://trac.webkit.org/r222058>.