Skip breakpoints on read-only bytecode pages instead of aborting

The debugger installs breakpoints by overwriting an opcode in the
bytecode stream with Debugger, which requires the page to be
writable. The patcher uses mprotect to make the page RW; on
failure it called hermes_fatal and aborted the process.

If the bytecode lives in a read-only segment (e.g. statically
linked into the binary as a const array, ending up in
__DATA_CONST / __TEXT_CONST on macOS), mprotect is rejected by
the OS (EACCES under hardened runtime) and the abort takes down
the whole process. This is reachable in practice because step /
restoration / on-load breakpoints install at offset 0 of every
CodeBlock about to execute, so stepping into such a CodeBlock
from anywhere kills the runtime.

Make the patch best-effort:

- CodeBlock::installBreakpointAtOffset and the static makeWritable
  helper now return bool. On failure no state is modified.
- Debugger::installBreakpoint returns BreakpointLocation* (nullptr
  on failure), rolling back the partial breakpointLocations_ entry
  it just inserted.
- doSetNonUserBreakpoint and setOnLoadBreakpoint silently skip when
  installBreakpoint fails. Step / restoration / on-load breakpoints
  are best-effort hints; the only loss is that stepping cannot
  pause at the entry of a CodeBlock whose page cannot be patched.
- setUserBreakpoint now returns bool so callers can propagate
  failure to their clients (e.g. CDP could surface a proper error
  back to DevTools instead of aborting).
- The four call sites that re-install a previously-installed
  breakpoint (the page is known writable since the original
  install succeeded) assert the result.

Author: tmikov

include/hermes/VM/CodeBlock.h

@@ -371,7 +371,11 @@ class CodeBlock final : private llvh::TrailingObjects<
   /// at location \p offset.
   /// Requires that there's a breakpoint registered at \p offset.
   /// Increments the user count of the associated runtime module.
-  void installBreakpointAtOffset(uint32_t offset);
+  /// \return true on success; false if the bytecode page cannot be made
+  ///   writable (e.g. statically embedded bytecode in a read-only segment
+  ///   that the OS refuses to remap, such as macOS __DATA_CONST under
+  ///   hardened runtime). On failure, no state is modified.
+  LLVM_NODISCARD bool installBreakpointAtOffset(uint32_t offset);
 
   /// Uninstalls the debugger instruction from the opcode stream
   /// at location \p offset, replacing it with \p opCode.

include/hermes/VM/Debugger/Debugger.h

@@ -473,8 +473,13 @@ class Debugger {
   /// for the given codeBlock and offset, else creates one.
   /// Used by other functions which should be called to set breakpoints.
   /// Installs a breakpoint at that location, doesn't modify it.
-  /// \return the location at which the breakpoint was installed.
-  BreakpointLocation &installBreakpoint(CodeBlock *codeBlock, uint32_t offset);
+  /// \return a pointer to the location at which the breakpoint was installed,
+  ///   or nullptr if the bytecode page cannot be made writable (e.g.
+  ///   statically embedded bytecode in a read-only segment that the OS
+  ///   refuses to remap). On failure, no state is modified.
+  LLVM_NODISCARD BreakpointLocation *installBreakpoint(
+      CodeBlock *codeBlock,
+      uint32_t offset);
 
   /// Helper function to uninstall a breakpoint. Always use this function to
   /// uninstall breakpoints. This takes care of the case when we purposely keep
@@ -489,7 +494,11 @@ class Debugger {
   /// If the physical breakpoint isn't enabled yet, patches the debugger
   /// instruction in.
   /// Sets the breakpoint ID to \p id.
-  void
+  /// \return true on success; false if the bytecode page cannot be made
+  ///   writable. On failure, no state is modified and the breakpoint is not
+  ///   physically installed (the caller may still keep it in
+  ///   userBreakpoints_ as a record).
+  bool
   setUserBreakpoint(CodeBlock *codeBlock, uint32_t offset, BreakpointID id);
 
   /// Should not be called directly except from \p setStepBreakpoint() or \p

lib/VM/CodeBlock.cpp

@@ -332,8 +332,10 @@ uint32_t CodeBlock::getVirtualOffset() const {
 #ifdef HERMES_ENABLE_DEBUGGER
 
 /// Makes the page that \p address is in writable.
-/// If it fails, aborts execution.
-static void makeWritable(void *address, size_t length) {
+/// \return true on success, false if the page cannot be made writable (e.g.
+///   the bytecode lives in a read-only segment of the binary that the OS
+///   refuses to remap, such as macOS __DATA_CONST under hardened runtime).
+static bool makeWritable(void *address, size_t length) {
   void *endAddress = static_cast<void *>(static_cast<char *>(address) + length);
 
   // Align the address to page size before setting the pagesize.
@@ -343,14 +345,11 @@ static void makeWritable(void *address, size_t length) {
   size_t totalLength =
       static_cast<char *>(endAddress) - static_cast<char *>(alignedAddress);
 
-  bool success = oscompat::vm_protect(
+  return oscompat::vm_protect(
       alignedAddress, totalLength, oscompat::ProtectMode::ReadWrite);
-  if (!success) {
-    hermes_fatal("mprotect failed before modifying breakpoint");
-  }
 }
 
-void CodeBlock::installBreakpointAtOffset(uint32_t offset) {
+bool CodeBlock::installBreakpointAtOffset(uint32_t offset) {
   auto opcodes = getOpcodeArray();
   assert(offset < opcodes.size() && "patch offset out of bounds");
   hbc::opcode_atom_t *address =
@@ -362,9 +361,12 @@ void CodeBlock::installBreakpointAtOffset(uint32_t offset) {
       sizeof(inst::DebuggerInst) == 1,
       "debugger instruction can only be a single opcode atom");
 
-  makeWritable(address, sizeof(inst::DebuggerInst));
+  if (!makeWritable(address, sizeof(inst::DebuggerInst))) {
+    return false;
+  }
   *address = debuggerOpcode;
   ++numInstalledBreakpoints_;
+  return true;
 }
 
 void CodeBlock::uninstallBreakpointAtOffset(

lib/VM/Debugger/Debugger.cpp

@@ -421,7 +421,12 @@ ExecutionStatus Debugger::debuggerLoop(
             }
             breakAtPossibleNextInstructions(state);
             if (breakpointOpt) {
-              state.codeBlock->installBreakpointAtOffset(state.offset);
+              // Re-installing a previously-installed breakpoint: the page is
+              // already known writable.
+              bool ok =
+                  state.codeBlock->installBreakpointAtOffset(state.offset);
+              (void)ok;
+              assert(ok && "re-installing existing breakpoint cannot fail");
             }
             if (stepMode == StepMode::Into) {
               // Stepping in could enter another code block,
@@ -767,18 +772,26 @@ Debugger::getBreakpointLocation(CodeBlock *codeBlock, uint32_t offset) const {
 }
 
 auto Debugger::installBreakpoint(CodeBlock *codeBlock, uint32_t offset)
-    -> BreakpointLocation & {
+    -> BreakpointLocation * {
   auto opcodes = codeBlock->getOpcodeArray();
   assert(offset < opcodes.size() && "invalid offset to set breakpoint");
-  auto &location =
-      breakpointLocations_
-          .try_emplace(codeBlock->getOffsetPtr(offset), opcodes[offset])
-          .first->second;
+  auto *opcodePtr = codeBlock->getOffsetPtr(offset);
+  auto emplaceResult =
+      breakpointLocations_.try_emplace(opcodePtr, opcodes[offset]);
+  auto &location = emplaceResult.first->second;
   if (location.count() == 0) {
     // count used to be 0, so patch this in now that the count > 0.
-    codeBlock->installBreakpointAtOffset(offset);
+    if (!codeBlock->installBreakpointAtOffset(offset)) {
+      // Patching failed (e.g. statically embedded bytecode in a read-only
+      // segment that the OS refuses to remap). Roll back the entry we just
+      // inserted so state stays consistent, and signal failure to caller.
+      if (emplaceResult.second) {
+        breakpointLocations_.erase(emplaceResult.first);
+      }
+      return nullptr;
+    }
   }
-  return location;
+  return &location;
 }
 
 void Debugger::uninstallBreakpoint(
@@ -796,28 +809,39 @@ void Debugger::uninstallBreakpoint(
   }
 }
 
-void Debugger::setUserBreakpoint(
+bool Debugger::setUserBreakpoint(
     CodeBlock *codeBlock,
     uint32_t offset,
     BreakpointID id) {
-  BreakpointLocation &location = installBreakpoint(codeBlock, offset);
-  location.userBreakpointIDs.insert(id);
+  BreakpointLocation *location = installBreakpoint(codeBlock, offset);
+  if (!location) {
+    return false;
+  }
+  location->userBreakpointIDs.insert(id);
+  return true;
 }
 
 void Debugger::doSetNonUserBreakpoint(
     CodeBlock *codeBlock,
     uint32_t offset,
     uint32_t callStackDepth,
     bool isStepBreakpoint) {
-  BreakpointLocation &location = installBreakpoint(codeBlock, offset);
+  BreakpointLocation *location = installBreakpoint(codeBlock, offset);
+  if (!location) {
+    // Best-effort: step/restoration breakpoints are an optimization for
+    // pause-on-entry while stepping. If the target lives in a read-only
+    // bytecode segment we cannot patch, just skip it -- stepping will simply
+    // not pause at the entry of that code block.
+    return;
+  }
   std::vector<Breakpoint> &breakpoints =
       isStepBreakpoint ? tempBreakpoints_ : restorationBreakpoints_;
-  if (location.callStackDepths.count(callStackDepth) == 0) {
-    location.callStackDepths.insert(callStackDepth);
+  if (location->callStackDepths.count(callStackDepth) == 0) {
+    location->callStackDepths.insert(callStackDepth);
   }
 
-  if ((isStepBreakpoint && !location.hasStepBreakpoint) ||
-      (!isStepBreakpoint && !location.hasRestorationBreakpoint)) {
+  if ((isStepBreakpoint && !location->hasStepBreakpoint) ||
+      (!isStepBreakpoint && !location->hasRestorationBreakpoint)) {
     // Leave the resolved location empty for now,
     // let the caller fill it in lazily.
     Breakpoint breakpoint{};
@@ -828,9 +852,9 @@ void Debugger::doSetNonUserBreakpoint(
   }
 
   if (isStepBreakpoint) {
-    location.hasStepBreakpoint = true;
+    location->hasStepBreakpoint = true;
   } else {
-    location.hasRestorationBreakpoint = true;
+    location->hasRestorationBreakpoint = true;
   }
 }
 
@@ -843,17 +867,22 @@ void Debugger::setStepBreakpoint(
 }
 
 void Debugger::setOnLoadBreakpoint(CodeBlock *codeBlock, uint32_t offset) {
-  BreakpointLocation &location = installBreakpoint(codeBlock, offset);
+  BreakpointLocation *location = installBreakpoint(codeBlock, offset);
+  if (!location) {
+    // Best-effort: pauseOnScriptLoad cannot pause inside a code block whose
+    // bytecode page is not writable. Skip.
+    return;
+  }
   // Leave the resolved location empty for now,
   // let the caller fill it in lazily.
   Breakpoint breakpoint{};
   breakpoint.codeBlock = codeBlock;
   breakpoint.offset = offset;
   breakpoint.enabled = true;
-  assert(!location.onLoad && "can't set duplicate on-load breakpoint");
-  location.onLoad = true;
+  assert(!location->onLoad && "can't set duplicate on-load breakpoint");
+  location->onLoad = true;
   tempBreakpoints_.push_back(breakpoint);
-  assert(location.count() && "invalid count following set breakpoint");
+  assert(location->count() && "invalid count following set breakpoint");
 }
 
 void Debugger::unsetUserBreakpoint(
@@ -1017,8 +1046,12 @@ void Debugger::setRestorationBreakpoint(
 
 bool Debugger::restoreBreakpointIfAny() {
   if (breakpointToRestore_.first != nullptr) {
-    breakpointToRestore_.first->installBreakpointAtOffset(
+    // Re-installing a previously-installed breakpoint: the page is already
+    // known writable.
+    bool ok = breakpointToRestore_.first->installBreakpointAtOffset(
         breakpointToRestore_.second);
+    (void)ok;
+    assert(ok && "re-installing existing breakpoint cannot fail");
     breakpointToRestore_ = {nullptr, 0};
     return true;
   }
@@ -1046,7 +1079,10 @@ ExecutionStatus Debugger::stepInstruction(InterpreterState &state) {
     // Temporarily uninstall the breakpoint so we can run the real instruction.
     uninstallBreakpoint(codeBlock, offset, locationOpt->opCode);
     status = runtime_.stepFunction(newState);
-    codeBlock->installBreakpointAtOffset(offset);
+    // Re-install: page already known writable.
+    bool ok = codeBlock->installBreakpointAtOffset(offset);
+    (void)ok;
+    assert(ok && "re-installing existing breakpoint cannot fail");
   } else {
     status = runtime_.stepFunction(newState);
   }
@@ -1092,7 +1128,10 @@ ExecutionStatus Debugger::processInstUnderDebuggerOpCode(
       runtime_.setCurrentIP(ip);
       ExecutionStatus status = runtime_.stepFunction(newState);
       runtime_.invalidateCurrentIP();
-      codeBlock->installBreakpointAtOffset(offset);
+      // Re-install: page already known writable.
+      bool ok = codeBlock->installBreakpointAtOffset(offset);
+      (void)ok;
+      assert(ok && "re-installing existing breakpoint cannot fail");
       if (status == ExecutionStatus::EXCEPTION) {
         return status;
       }