Skip to content

Commit 91f70aa

Browse files
author
Mark Lam
committed
Enhance sanitizeStackForVM() to assist with crash analysis.
https://bugs.webkit.org/show_bug.cgi?id=235752 rdar://81014601 Reviewed by Michael Saboff. Source/JavaScriptCore: 1. Remove the AssemblyHelpers version of sanitizeStack. Instead, make the 3 JIT operation functions call sanitizeStackForVM() instead. This ensures that sanitizeStack crashes are not obscured as generic JIT crashes. 2. Add sanity check RELEASE_ASSERTs to VM::setLastStackTop() with a capture of the relevant variables for crash analysis. 3. Fix logSanitizeStack() so that it no longer relies on vm.topCallFrame. vm.topCallFrame may not be properly initialized at all the places that sanitizeStackForVM() is called. 4. Add a JSLock check to sanitizeStackForVM(), and return early if not owned by the current thread. If the JSLock is not owned by the current thread, we can't rely on vm.lastStackTop() being a sane value. Hence, it's not possible to do stack sanitization correctly. Add sanity check RELEASE_ASSERTs to sanitizeStackForVM() with a capture of the relevant variables for crash analysis. * jit/AssemblyHelpers.cpp: (JSC::AssemblyHelpers::sanitizeStackInline): Deleted. * jit/AssemblyHelpers.h: * jit/JITOperations.cpp: (JSC::JSC_DEFINE_JIT_OPERATION): * jit/ThunkGenerators.cpp: (JSC::slowPathFor): * runtime/JSLock.cpp: (JSC::JSLock::didAcquireLock): (JSC::JSLock::grabAllLocks): * runtime/VM.cpp: (JSC::VM::VM): (JSC::VM::setLastStackTop): (JSC::logSanitizeStack): (JSC::sanitizeStackForVM): * runtime/VM.h: Source/WTF: * wtf/Threading.h: (WTF::Thread::savedStackPointerAtVMEntry const): (WTF::Thread::savedLastStackTop const): (WTF::Thread::savedStackPointerAtVMEntry): Deleted. (WTF::Thread::savedLastStackTop): Deleted. Canonical link: https://commits.webkit.org/246637@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288885 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent b67ad67 commit 91f70aa

10 files changed

Lines changed: 90 additions & 48 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,47 @@
1+
2022-02-01 Mark Lam <mark.lam@apple.com>
2+
3+
Enhance sanitizeStackForVM() to assist with crash analysis.
4+
https://bugs.webkit.org/show_bug.cgi?id=235752
5+
rdar://81014601
6+
7+
Reviewed by Michael Saboff.
8+
9+
1. Remove the AssemblyHelpers version of sanitizeStack. Instead, make the 3
10+
JIT operation functions call sanitizeStackForVM() instead. This ensures
11+
that sanitizeStack crashes are not obscured as generic JIT crashes.
12+
13+
2. Add sanity check RELEASE_ASSERTs to VM::setLastStackTop() with a capture of
14+
the relevant variables for crash analysis.
15+
16+
3. Fix logSanitizeStack() so that it no longer relies on vm.topCallFrame.
17+
vm.topCallFrame may not be properly initialized at all the places that
18+
sanitizeStackForVM() is called.
19+
20+
4. Add a JSLock check to sanitizeStackForVM(), and return early if not owned by
21+
the current thread. If the JSLock is not owned by the current thread, we can't
22+
rely on vm.lastStackTop() being a sane value. Hence, it's not possible to
23+
do stack sanitization correctly.
24+
25+
Add sanity check RELEASE_ASSERTs to sanitizeStackForVM() with a capture of
26+
the relevant variables for crash analysis.
27+
28+
* jit/AssemblyHelpers.cpp:
29+
(JSC::AssemblyHelpers::sanitizeStackInline): Deleted.
30+
* jit/AssemblyHelpers.h:
31+
* jit/JITOperations.cpp:
32+
(JSC::JSC_DEFINE_JIT_OPERATION):
33+
* jit/ThunkGenerators.cpp:
34+
(JSC::slowPathFor):
35+
* runtime/JSLock.cpp:
36+
(JSC::JSLock::didAcquireLock):
37+
(JSC::JSLock::grabAllLocks):
38+
* runtime/VM.cpp:
39+
(JSC::VM::VM):
40+
(JSC::VM::setLastStackTop):
41+
(JSC::logSanitizeStack):
42+
(JSC::sanitizeStackForVM):
43+
* runtime/VM.h:
44+
145
2022-01-31 Yusuke Suzuki <ysuzuki@apple.com>
246

347
Revert OSAllocator behavior to pre-Structure-Allocator change one

‎Source/JavaScriptCore/jit/AssemblyHelpers.cpp‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2011-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2011-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -1113,19 +1113,6 @@ void AssemblyHelpers::copyCalleeSavesToEntryFrameCalleeSavesBufferImpl(GPRReg ca
11131113
#endif
11141114
}
11151115

1116-
void AssemblyHelpers::sanitizeStackInline(VM& vm, GPRReg scratch)
1117-
{
1118-
loadPtr(vm.addressOfLastStackTop(), scratch);
1119-
Jump done = branchPtr(BelowOrEqual, stackPointerRegister, scratch);
1120-
Label loop = label();
1121-
storePtr(TrustedImmPtr(nullptr), Address(scratch));
1122-
addPtr(TrustedImmPtr(sizeof(void*)), scratch);
1123-
branchPtr(Above, stackPointerRegister, scratch).linkTo(loop, this);
1124-
done.link(this);
1125-
move(stackPointerRegister, scratch);
1126-
storePtr(scratch, vm.addressOfLastStackTop());
1127-
}
1128-
11291116
void AssemblyHelpers::cageWithoutUntagging(Gigacage::Kind kind, GPRReg storage)
11301117
{
11311118
#if GIGACAGE_ENABLED

‎Source/JavaScriptCore/jit/AssemblyHelpers.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2011-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2011-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -1710,8 +1710,6 @@ class AssemblyHelpers : public MacroAssembler {
17101710
return branchTest8(Zero, AbsoluteAddress(vm.heap.addressOfMutatorShouldBeFenced()));
17111711
}
17121712

1713-
void sanitizeStackInline(VM&, GPRReg scratch);
1714-
17151713
// Emits the branch structure for typeof. The code emitted by this doesn't fall through. The
17161714
// functor is called at those points where we have pinpointed a type. One way to use this is to
17171715
// have the functor emit the code to put the type string into an appropriate register and then

‎Source/JavaScriptCore/jit/JITOperations.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2013-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2013-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -1448,11 +1448,13 @@ JSC_DEFINE_JIT_OPERATION(operationCallEval, EncodedJSValue, (JSGlobalObject* glo
14481448

14491449
JSC_DEFINE_JIT_OPERATION(operationLinkCall, SlowPathReturnType, (CallFrame* calleeFrame, JSGlobalObject* globalObject, CallLinkInfo* callLinkInfo))
14501450
{
1451+
sanitizeStackForVM(globalObject->vm());
14511452
return linkFor(calleeFrame, globalObject, callLinkInfo);
14521453
}
14531454

14541455
JSC_DEFINE_JIT_OPERATION(operationLinkPolymorphicCall, SlowPathReturnType, (CallFrame* calleeFrame, JSGlobalObject* globalObject, CallLinkInfo* callLinkInfo))
14551456
{
1457+
sanitizeStackForVM(globalObject->vm());
14561458
ASSERT(callLinkInfo->specializationKind() == CodeForCall);
14571459
JSCell* calleeAsFunctionCell;
14581460
SlowPathReturnType result = virtualForWithFunction(globalObject, calleeFrame, callLinkInfo, calleeAsFunctionCell);
@@ -1464,6 +1466,7 @@ JSC_DEFINE_JIT_OPERATION(operationLinkPolymorphicCall, SlowPathReturnType, (Call
14641466

14651467
JSC_DEFINE_JIT_OPERATION(operationVirtualCall, SlowPathReturnType, (CallFrame* calleeFrame, JSGlobalObject* globalObject, CallLinkInfo* callLinkInfo))
14661468
{
1469+
sanitizeStackForVM(globalObject->vm());
14671470
JSCell* calleeAsFunctionCellIgnored;
14681471
return virtualForWithFunction(globalObject, calleeFrame, callLinkInfo, calleeAsFunctionCellIgnored);
14691472
}

‎Source/JavaScriptCore/jit/ThunkGenerators.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2010-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2010-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -192,7 +192,6 @@ MacroAssemblerCodeRef<JITThunkPtrTag> throwExceptionFromCallSlowPathGenerator(VM
192192

193193
static void slowPathFor(CCallHelpers& jit, VM& vm, Sprt_JITOperation_EGCli slowPathFunction)
194194
{
195-
jit.sanitizeStackInline(vm, GPRInfo::nonArgGPR0);
196195
jit.emitFunctionPrologue();
197196
jit.storePtr(GPRInfo::callFrameRegister, &vm.topCallFrame);
198197
#if OS(WINDOWS) && CPU(X86_64)

‎Source/JavaScriptCore/runtime/JSLock.cpp‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2005-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2005-2022 Apple Inc. All rights reserved.
33
*
44
* This library is free software; you can redistribute it and/or
55
* modify it under the terms of the GNU Library General Public
@@ -138,8 +138,7 @@ void JSLock::didAcquireLock()
138138
m_entryAtomStringTable = thread.setCurrentAtomStringTable(m_vm->atomStringTable());
139139
ASSERT(m_entryAtomStringTable);
140140

141-
m_vm->setLastStackTop(thread.savedLastStackTop());
142-
ASSERT(thread.stack().contains(m_vm->lastStackTop()));
141+
m_vm->setLastStackTop(thread);
143142

144143
if (m_vm->heap.hasAccess())
145144
m_shouldReleaseHeapAccess = false;
@@ -277,7 +276,7 @@ void JSLock::grabAllLocks(DropAllLocks* dropper, unsigned droppedLockCount)
277276

278277
Thread& thread = Thread::current();
279278
m_vm->setStackPointerAtVMEntry(thread.savedStackPointerAtVMEntry());
280-
m_vm->setLastStackTop(thread.savedLastStackTop());
279+
m_vm->setLastStackTop(thread);
281280
}
282281

283282
JSLock::DropAllLocks::DropAllLocks(VM* vm)

‎Source/JavaScriptCore/runtime/VM.cpp‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2008-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2008-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -242,9 +242,8 @@ VM::VM(VMType vmType, HeapType heapType, WTF::RunLoop* runLoop, bool* success)
242242
CRASH_WITH_INFO(0x4242424220202020, 0xbadbeef0badbeef, 0x1234123412341234, 0x1337133713371337);
243243

244244
interpreter = new Interpreter(*this);
245-
StackBounds stack = Thread::current().stack();
246245
updateSoftReservedZoneSize(Options::softReservedZoneSize());
247-
setLastStackTop(stack.origin());
246+
setLastStackTop(Thread::current());
248247

249248
JSRunLoopTimer::Manager::shared().registerVM(*this);
250249

@@ -503,9 +502,11 @@ void VM::primitiveGigacageDisabled()
503502
m_needToFirePrimitiveGigacageEnabled = true;
504503
}
505504

506-
void VM::setLastStackTop(void* lastStackTop)
507-
{
508-
m_lastStackTop = lastStackTop;
505+
void VM::setLastStackTop(const Thread& thread)
506+
{
507+
m_lastStackTop = thread.savedLastStackTop();
508+
auto& stack = thread.stack();
509+
RELEASE_ASSERT(stack.contains(m_lastStackTop), 0x5510, m_lastStackTop, stack.origin(), stack.end());
509510
}
510511

511512
Ref<VM> VM::createContextGroup(HeapType heapType)
@@ -1050,16 +1051,11 @@ void VM::popAllCheckpointOSRSideStateUntil(CallFrame* target)
10501051
m_checkpointSideState.shrinkToFit();
10511052
}
10521053

1053-
void logSanitizeStack(VM& vm)
1054+
static void logSanitizeStack(VM& vm)
10541055
{
1055-
if (Options::verboseSanitizeStack() && vm.topCallFrame) {
1056-
int dummy;
1056+
if (UNLIKELY(Options::verboseSanitizeStack())) {
10571057
auto& stackBounds = Thread::current().stack();
1058-
dataLog(
1059-
"Sanitizing stack for VM = ", RawPointer(&vm), " with top call frame at ", RawPointer(vm.topCallFrame),
1060-
", current stack pointer at ", RawPointer(&dummy), ", in ",
1061-
pointerDump(vm.topCallFrame->codeBlock()), ", last code origin = ",
1062-
vm.topCallFrame->codeOrigin(), ", last stack top = ", RawPointer(vm.lastStackTop()), ", in stack range [", RawPointer(stackBounds.origin()), ", ", RawPointer(stackBounds.end()), "]\n");
1058+
dataLogLn("Sanitizing stack for VM = ", RawPointer(&vm), ", current stack pointer at ", RawPointer(currentStackPointer()), ", last stack top = ", RawPointer(vm.lastStackTop()), ", in stack range (", RawPointer(stackBounds.end()), ", ", RawPointer(stackBounds.origin()), "]");
10631059
}
10641060
}
10651061

@@ -1270,17 +1266,20 @@ void QueuedTask::run()
12701266

12711267
void sanitizeStackForVM(VM& vm)
12721268
{
1269+
auto& thread = Thread::current();
1270+
auto& stack = thread.stack();
1271+
if (!vm.currentThreadIsHoldingAPILock())
1272+
return; // vm.lastStackTop() may not be set up correctly if JSLock is not held.
1273+
12731274
logSanitizeStack(vm);
1274-
if (vm.topCallFrame) {
1275-
auto& stackBounds = Thread::current().stack();
1276-
ASSERT(vm.currentThreadIsHoldingAPILock());
1277-
ASSERT_UNUSED(stackBounds, stackBounds.contains(vm.lastStackTop()));
1278-
}
1275+
1276+
RELEASE_ASSERT(stack.contains(vm.lastStackTop()), 0xaa10, vm.lastStackTop(), stack.origin(), stack.end());
12791277
#if ENABLE(C_LOOP)
12801278
vm.interpreter->cloopStack().sanitizeStack();
12811279
#else
12821280
sanitizeStackForVMImpl(&vm);
12831281
#endif
1282+
RELEASE_ASSERT(stack.contains(vm.lastStackTop()), 0xaa20, vm.lastStackTop(), stack.origin(), stack.end());
12841283
}
12851284

12861285
size_t VM::committedStackByteCount()

‎Source/JavaScriptCore/runtime/VM.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2008-2021 Apple Inc. All rights reserved.
2+
* Copyright (C) 2008-2022 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -774,7 +774,7 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
774774

775775
void** addressOfLastStackTop() { return &m_lastStackTop; }
776776
void* lastStackTop() { return m_lastStackTop; }
777-
void setLastStackTop(void*);
777+
void setLastStackTop(const Thread&);
778778

779779
void firePrimitiveGigacageEnabledIfNecessary()
780780
{
@@ -1162,6 +1162,5 @@ extern "C" void sanitizeStackForVMImpl(VM*);
11621162
#endif
11631163

11641164
JS_EXPORT_PRIVATE void sanitizeStackForVM(VM&);
1165-
void logSanitizeStack(VM&);
11661165

11671166
} // namespace JSC

‎Source/WTF/ChangeLog‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2022-02-01 Mark Lam <mark.lam@apple.com>
2+
3+
Enhance sanitizeStackForVM() to assist with crash analysis.
4+
https://bugs.webkit.org/show_bug.cgi?id=235752
5+
rdar://81014601
6+
7+
Reviewed by Michael Saboff.
8+
9+
* wtf/Threading.h:
10+
(WTF::Thread::savedStackPointerAtVMEntry const):
11+
(WTF::Thread::savedLastStackTop const):
12+
(WTF::Thread::savedStackPointerAtVMEntry): Deleted.
13+
(WTF::Thread::savedLastStackTop): Deleted.
14+
115
2022-01-31 Yusuke Suzuki <ysuzuki@apple.com>
216

317
Revert OSAllocator behavior to pre-Structure-Allocator change one

‎Source/WTF/wtf/Threading.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ class Thread : public ThreadSafeRefCounted<Thread> {
231231
}
232232
#endif
233233

234-
void* savedStackPointerAtVMEntry()
234+
void* savedStackPointerAtVMEntry() const
235235
{
236236
return m_savedStackPointerAtVMEntry;
237237
}
@@ -241,7 +241,7 @@ class Thread : public ThreadSafeRefCounted<Thread> {
241241
m_savedStackPointerAtVMEntry = stackPointerAtVMEntry;
242242
}
243243

244-
void* savedLastStackTop()
244+
void* savedLastStackTop() const
245245
{
246246
return m_savedLastStackTop;
247247
}

0 commit comments

Comments
 (0)