Attachment 346410 Details for Bug 188271 – patch

[patch] patch

b-backup.diff (text/plain), 13.31 KB, created by Saam Barati on 2018-08-02 13:05:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-08-02 13:05:42 PDT

Size: 13.31 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 234508)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,31 @@
>+2018-08-02  Saam Barati  <sbarati@apple.com>
>+
>+        Reading instructionPointer from PlatformRegisters may fail when using pointer profiling
>+        https://bugs.webkit.org/show_bug.cgi?id=188271
>+        <rdar://problem/42850884>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch defends against the instructionPointer containing garbage bits.
>+        See radar for details.
>+
>+        * runtime/MachineContext.h:
>+        (JSC::MachineContext::instructionPointer):
>+        * runtime/SamplingProfiler.cpp:
>+        (JSC::SamplingProfiler::takeSample):
>+        * runtime/VMTraps.cpp:
>+        (JSC::SignalContext::SignalContext):
>+        (JSC::SignalContext::tryCreate):
>+        * tools/CodeProfiling.cpp:
>+        (JSC::profilingTimer):
>+        * tools/SigillCrashAnalyzer.cpp:
>+        (JSC::SignalContext::SignalContext):
>+        (JSC::SignalContext::tryCreate):
>+        (JSC::SignalContext::dump):
>+        (JSC::installCrashHandler):
>+        * wasm/WasmFaultSignalHandler.cpp:
>+        (JSC::Wasm::trapHandler):
>+
> 2018-08-02  David Fenton  <david_fenton@apple.com>
> 
>         Unreviewed, rolling out r234489.
>Index: Source/JavaScriptCore/runtime/MachineContext.h
>===================================================================
>--- Source/JavaScriptCore/runtime/MachineContext.h	(revision 234503)
>+++ Source/JavaScriptCore/runtime/MachineContext.h	(working copy)
>@@ -29,6 +29,7 @@
> #include "GPRInfo.h"
> #include "LLIntPCRanges.h"
> #include "MacroAssemblerCodeRef.h"
>+#include <wtf/Optional.h>
> #include <wtf/PlatformRegisters.h>
> #include <wtf/PointerPreparations.h>
> #include <wtf/StdLibExtras.h>
>@@ -44,7 +45,7 @@ template<typename T = void*> T framePoin
> template<typename T = void*> void setFramePointer(PlatformRegisters&, T);
> inline MacroAssemblerCodePtr<CFunctionPtrTag> linkRegister(const PlatformRegisters&);
> inline void setLinkRegister(PlatformRegisters&, MacroAssemblerCodePtr<CFunctionPtrTag>);
>-inline MacroAssemblerCodePtr<CFunctionPtrTag> instructionPointer(const PlatformRegisters&);
>+inline std::optional<MacroAssemblerCodePtr<CFunctionPtrTag>> instructionPointer(const PlatformRegisters&);
> inline void setInstructionPointer(PlatformRegisters&, MacroAssemblerCodePtr<CFunctionPtrTag>);
> 
> template<size_t N> void*& argumentPointer(PlatformRegisters&);
>@@ -431,7 +432,7 @@ static inline void*& instructionPointerI
> }
> #endif // !USE(PLATFORM_REGISTERS_WITH_PROFILE)
> 
>-inline MacroAssemblerCodePtr<CFunctionPtrTag> instructionPointer(const PlatformRegisters& regs)
>+inline std::optional<MacroAssemblerCodePtr<CFunctionPtrTag>> instructionPointer(const PlatformRegisters& regs)
> {
> #if USE(PLATFORM_REGISTERS_WITH_PROFILE)
>     void* value = WTF_READ_PLATFORM_REGISTERS_PC_WITH_PROFILE(regs);
>@@ -440,7 +441,11 @@ inline MacroAssemblerCodePtr<CFunctionPt
> #endif
>     if (!value)
>         return MacroAssemblerCodePtr<CFunctionPtrTag>(nullptr);
>-    return MacroAssemblerCodePtr<CFunctionPtrTag>(value);
>+    if (!usesPointerTagging())
>+        return MacroAssemblerCodePtr<CFunctionPtrTag>(value);
>+    if (isTaggedWith(value, CFunctionPtrTag))
>+        return MacroAssemblerCodePtr<CFunctionPtrTag>(value);
>+    return std::nullopt;
> }
> 
> inline void setInstructionPointer(PlatformRegisters& regs, MacroAssemblerCodePtr<CFunctionPtrTag> value)
>Index: Source/JavaScriptCore/runtime/SamplingProfiler.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/SamplingProfiler.cpp	(revision 234503)
>+++ Source/JavaScriptCore/runtime/SamplingProfiler.cpp	(working copy)
>@@ -356,7 +356,11 @@ void SamplingProfiler::takeSample(const 
>                 m_jscExecutionThread->getRegisters(registers);
>                 machineFrame = MachineContext::framePointer(registers);
>                 callFrame = static_cast<ExecState*>(machineFrame);
>-                machinePC = MachineContext::instructionPointer(registers).untaggedExecutableAddress();
>+                auto instructionPointer = MachineContext::instructionPointer(registers);
>+                if (instructionPointer)
>+                    machinePC = instructionPointer->untaggedExecutableAddress();
>+                else
>+                    machinePC = nullptr;
>                 llintPC = removeCodePtrTag(MachineContext::llintInstructionPointer(registers));
>                 assertIsNotTagged(machinePC);
>             }
>Index: Source/JavaScriptCore/runtime/VMTraps.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VMTraps.cpp	(revision 234503)
>+++ Source/JavaScriptCore/runtime/VMTraps.cpp	(working copy)
>@@ -55,13 +55,23 @@ ALWAYS_INLINE VM& VMTraps::vm() const
> #if ENABLE(SIGNAL_BASED_VM_TRAPS)
> 
> struct SignalContext {
>-    SignalContext(PlatformRegisters& registers)
>+private:
>+    SignalContext(PlatformRegisters& registers, MacroAssemblerCodePtr<CFunctionPtrTag> trapPC)
>         : registers(registers)
>-        , trapPC(MachineContext::instructionPointer(registers))
>+        , trapPC(trapPC)
>         , stackPointer(MachineContext::stackPointer(registers))
>         , framePointer(MachineContext::framePointer(registers))
>     { }
> 
>+public:
>+    static std::optional<SignalContext> tryCreate(PlatformRegisters& registers)
>+    {
>+        auto instructionPointer = MachineContext::instructionPointer(registers);
>+        if (!instructionPointer)
>+            return std::nullopt;
>+        return SignalContext(registers, *instructionPointer);
>+    }
>+
>     PlatformRegisters& registers;
>     MacroAssemblerCodePtr<CFunctionPtrTag> trapPC;
>     void* stackPointer;
>@@ -186,9 +196,11 @@ public:
>         static std::once_flag once;
>         std::call_once(once, [] {
>             installSignalHandler(Signal::BadAccess, [] (Signal, SigInfo&, PlatformRegisters& registers) -> SignalAction {
>-                SignalContext context(registers);
>+                auto signalContext = SignalContext::tryCreate(registers);
>+                if (!signalContext)
>+                    return SignalAction::NotHandled;
> 
>-                void* trapPC = context.trapPC.untaggedExecutableAddress();
>+                void* trapPC = signalContext->trapPC.untaggedExecutableAddress();
>                 if (!isJITPC(trapPC))
>                     return SignalAction::NotHandled;
> 
>@@ -249,7 +261,9 @@ protected:
>         auto optionalOwnerThread = vm.ownerThread();
>         if (optionalOwnerThread) {
>             sendMessage(*optionalOwnerThread.value().get(), [&] (PlatformRegisters& registers) -> void {
>-                SignalContext context(registers);
>+                auto signalContext = SignalContext::tryCreate(registers);
>+                if (!signalContext)
>+                    return;
> 
>                 auto ownerThread = vm.apiLock().ownerThread();
>                 // We can't mess with a thread unless it's the one we suspended.
>@@ -257,7 +271,7 @@ protected:
>                     return;
> 
>                 Thread& thread = *ownerThread->get();
>-                vm.traps().tryInstallTrapBreakpoints(context, thread.stack());
>+                vm.traps().tryInstallTrapBreakpoints(*signalContext, thread.stack());
>             });
>         }
> 
>Index: Source/JavaScriptCore/tools/CodeProfiling.cpp
>===================================================================
>--- Source/JavaScriptCore/tools/CodeProfiling.cpp	(revision 234503)
>+++ Source/JavaScriptCore/tools/CodeProfiling.cpp	(working copy)
>@@ -71,9 +71,11 @@ static void setProfileTimer(unsigned use
> static void profilingTimer(int, siginfo_t*, void* uap)
> {
>     PlatformRegisters& platformRegisters = WTF::registersFromUContext(static_cast<ucontext_t*>(uap));
>-    CodeProfiling::sample(
>-        MachineContext::instructionPointer(platformRegisters).untaggedExecutableAddress(),
>-        reinterpret_cast<void**>(MachineContext::framePointer(platformRegisters)));
>+    if (auto instructionPointer = MachineContext::instructionPointer(platformRegisters)) {
>+        CodeProfiling::sample(
>+            instructionPointer->untaggedExecutableAddress(),
>+            reinterpret_cast<void**>(MachineContext::framePointer(platformRegisters)));
>+    }
> }
> #endif
> 
>Index: Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp
>===================================================================
>--- Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp	(revision 234503)
>+++ Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp	(working copy)
>@@ -78,13 +78,23 @@ private:
> #endif // USE(OS_LOG)
> 
> struct SignalContext {
>-    SignalContext(PlatformRegisters& registers)
>+private:
>+    SignalContext(PlatformRegisters& registers, MacroAssemblerCodePtr<CFunctionPtrTag> machinePC)
>         : registers(registers)
>-        , machinePC(MachineContext::instructionPointer(registers))
>+        , machinePC(machinePC)
>         , stackPointer(MachineContext::stackPointer(registers))
>         , framePointer(MachineContext::framePointer(registers))
>     { }
> 
>+public:
>+    static std::optional<SignalContext> tryCreate(PlatformRegisters& registers)
>+    {
>+        auto instructionPointer = MachineContext::instructionPointer(registers);
>+        if (!instructionPointer)
>+            return std::nullopt;
>+        return SignalContext(registers, *instructionPointer);
>+    }
>+
>     void dump()
>     {
> #if CPU(X86_64)
>@@ -132,7 +142,7 @@ struct SignalContext {
>             MachineContext::linkRegister(registers).untaggedExecutableAddress<uint64_t>());
>         log("sp: %016llx pc: %016llx cpsr: %08x",
>             MachineContext::stackPointer<uint64_t>(registers),
>-            MachineContext::instructionPointer(registers).untaggedExecutableAddress<uint64_t>(),
>+            machinePC.untaggedExecutableAddress<uint64_t>(),
>             registers.__cpsr);
> #endif
>     }
>@@ -147,14 +157,16 @@ static void installCrashHandler()
> {
> #if CPU(X86_64) || CPU(ARM64)
>     installSignalHandler(Signal::Ill, [] (Signal, SigInfo&, PlatformRegisters& registers) {
>-        SignalContext context(registers);
>-
>-        void* machinePC = context.machinePC.untaggedExecutableAddress();
>+        auto signalContext = SignalContext::tryCreate(registers);
>+        if (!signalContext)
>+            return SignalAction::NotHandled;
>+            
>+        void* machinePC = signalContext->machinePC.untaggedExecutableAddress();
>         if (!isJITPC(machinePC))
>             return SignalAction::NotHandled;
> 
>         SigillCrashAnalyzer& analyzer = SigillCrashAnalyzer::instance();
>-        analyzer.analyze(context);
>+        analyzer.analyze(*signalContext);
>         return SignalAction::NotHandled;
>     });
> #endif
>Index: Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp
>===================================================================
>--- Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp	(revision 234503)
>+++ Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp	(working copy)
>@@ -56,7 +56,10 @@ static bool fastHandlerInstalled { false
> 
> static SignalAction trapHandler(Signal, SigInfo& sigInfo, PlatformRegisters& context)
> {
>-    void* faultingInstruction = MachineContext::instructionPointer(context).untaggedExecutableAddress();
>+    auto instructionPointer = MachineContext::instructionPointer(context);
>+    if (!instructionPointer)
>+        return SignalAction::NotHandled;
>+    void* faultingInstruction = instructionPointer->untaggedExecutableAddress();
>     dataLogLnIf(WasmFaultSignalHandlerInternal::verbose, "starting handler for fault at: ", RawPointer(faultingInstruction));
> 
>     dataLogLnIf(WasmFaultSignalHandlerInternal::verbose, "JIT memory start: ", RawPointer(startOfFixedExecutableMemoryPool()), " end: ", RawPointer(endOfFixedExecutableMemoryPool()));
>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 234503)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-08-02  Saam Barati  <sbarati@apple.com>
>+
>+        Reading instructionPointer from PlatformRegisters may fail when using pointer tagging
>+        https://bugs.webkit.org/show_bug.cgi?id=188271
>+        <rdar://problem/42850884>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * wtf/PtrTag.h:
>+        (WTF::isTaggedWith):
>+        (WTF::usesPointerTagging):
>+
> 2018-08-02  David Fenton  <david_fenton@apple.com>
> 
>         Unreviewed, rolling out r234489.
>Index: Source/WTF/wtf/PtrTag.h
>===================================================================
>--- Source/WTF/wtf/PtrTag.h	(revision 234503)
>+++ Source/WTF/wtf/PtrTag.h	(working copy)
>@@ -154,9 +154,13 @@ template<typename PtrType> void assertIs
> template<typename PtrType> void assertIsTagged(PtrType) { }
> template<typename PtrType> void assertIsNullOrTagged(PtrType) { }
> 
>+template<typename PtrType> bool isTaggedWith(PtrType, PtrTag) { return false; }
>+
> template<typename PtrType> void assertIsTaggedWith(PtrType, PtrTag) { }
> template<typename PtrType> void assertIsNullOrTaggedWith(PtrType, PtrTag) { }
> 
>+inline bool usesPointerTagging() { return false; }
>+
> #define CALL_WITH_PTRTAG(callInstructionString, targetRegisterString, tag) \
>     callInstructionString " " targetRegisterString "\n"
> 
>@@ -186,5 +190,7 @@ using WTF::assertIsNullOrCFunctionPtr;
> using WTF::assertIsNotTagged;
> using WTF::assertIsTagged;
> using WTF::assertIsNullOrTagged;
>+using WTF::isTaggedWith;
> using WTF::assertIsTaggedWith;
> using WTF::assertIsNullOrTaggedWith;
>+using WTF::usesPointerTagging;

Actions: View | Formatted Diff | Diff

Attachments on bug 188271: 346410