Bug 176874 – AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Renata Hodovan]

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

[Radar WebKit Bug Importer]

<rdar://problem/34417206>

[Mark Lam]

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.

[Mark Lam]

Created attachment 320712 [details]
proposed patch.

[Keith Miller]

Comment on attachment 320712 [details]
proposed patch.

r=me.

[Mark Lam]

Created attachment 320715 [details]
proposed patch.

Saam pointed out that I also need to suppress ASan on Page::flushWrites() as well.

[Alexey Proskuryakov]

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

[Mark Lam]

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 ***

[Mark Lam]

Actually, this is a Probe implementation issue, and not a DFG OSR exit issue.  I'll fix it right away.

[Mark Lam]

<rdar://problem/34436415>

[Mark Lam]

Created attachment 320822 [details]
proposed patch.

[Build Bot]

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.

[Saam Barati]

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

[Mark Lam]

Comment on attachment 320822 [details]
proposed patch.

Will apply Saam's feedback.

[Mark Lam]

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.

[Mark Lam]

Created attachment 320832 [details]
patch for landing.

[Build Bot]

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.

[Mark Lam]

Thanks for the review.  Landed in r222058: <http://trac.webkit.org/r222058>.