[webkit-changes] [WebKit/WebKit] 5b5a2d: WTFReportBacktrace fails to print backtrace and cr...

[Kimmo Kinnunen]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 5b5a2dc989fa65afa3eef1c084b690a01fa17813
      https://github.com/WebKit/WebKit/commit/5b5a2dc989fa65afa3eef1c084b690a01fa17813
  Author: Kimmo Kinnunen <kkinnunen at apple.com>
  Date:   2022-10-05 (Wed, 05 Oct 2022)

  Changed paths:
    M Source/JavaScriptCore/b3/B3Value.cpp
    M Source/JavaScriptCore/heap/VerifierSlotVisitor.cpp
    M Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp
    M Source/JavaScriptCore/runtime/ExceptionScope.cpp
    M Source/JavaScriptCore/runtime/SamplingProfiler.cpp
    M Source/JavaScriptCore/runtime/VM.cpp
    M Source/WTF/wtf/Assertions.cpp
    M Source/WTF/wtf/Assertions.h
    M Source/WTF/wtf/StackCheck.cpp
    M Source/WTF/wtf/StackShotProfiler.h
    M Source/WTF/wtf/StackTrace.cpp
    M Source/WTF/wtf/StackTrace.h
    M Source/WTF/wtf/win/DbgHelperWin.h
    M Tools/TestWebKitAPI/CMakeLists.txt
    M Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
    A Tools/TestWebKitAPI/Tests/WTF/StackTraceTest.cpp

  Log Message:
  -----------
  WTFReportBacktrace fails to print backtrace and crashes on ASAN
https://bugs.webkit.org/show_bug.cgi?id=245826
rdar://problem/100557350

Reviewed by Mark Lam and Yusuke Suzuki.

WTFReportBacktrace() would use StackTrace in the "borrowed" mode.
The borrowed mode stack pointer would be obtained by reading
uninitialized !m_capacity. This would cause read to random memory.

Fix by removing the StackTrace borrowed mode. When printing
the backtrace, use the new class StackTracePrinter which
is able to use the void* array WTFGetStacktrace returns.

Move the size/isBorrowed members out of the union that was
intended to optimize skipping of the two initial, skipped
frames. This was relying on two counts of UB:
- reading of non-active member of the union:
  Filled the m_stack by writing into one union m_skippedFrame0
  making it active, but then reading from passive union
  member m_size.

- Writing into one member, and expecting the overflowing
  of the write being defined to match the subsequent member:
  Oversized write into union member m_skippedFrame0
  was assumend to spill over to member of other, subsequent union
  member m_stack. This is not well-defined.

The above UB invocations were done to trying to save two
void* worth of memory in the StackTrace, while avoiding
copying of the void* array.

Instead, first read the stack trace into the memory storage
of StackTrace, then overwrite the initial portion with the
real class data.

Fix bugs in the unborrowed mode, where
captureStackTrace(maxFrames, framesToSkip)
would not actually skip the framesToSkip amount of frames.
If left unfixed, we could not assert that the borrowed
mode fix did not accidentally cause regressions to the
unborrowed mode.

Move the printing related StackTrace::m_prefix to
StackTracePrinter::m_prefix. This way StackTrace size shrinks.

Test the equivalent of WTFReportBacktrace in a scenario
that caused the uninitialized read.

Test the captureStackTrace().

(WTF::StackTrace::instanceSize):
(WTF::StackTrace::captureStackTrace):
* Source/WTF/wtf/StackTrace.h:
(WTF::StackTrace::StackTrace):
(WTF::StackTrace::stack const):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WTF/StackTraceTest.cpp: Added.
(TestWebKitAPI::TEST):
(TestWebKitAPI::expectEqualFrames):

Canonical link: https://commits.webkit.org/255164@main