From 73b918cfcdb38b4472b118f00e5649309013b202 Mon Sep 17 00:00:00 2001
From: Simon Zünd <szuend@chromium.org>
Date: Tue, 15 Apr 2025 04:33:38 +0000
Subject: [PATCH] [inspector] Add V8Inspector::connectShared

This CL adds a version of the 'connect' method that returns a shared_ptr
rather than a unique_ptr.

This allows us to defer deconstruction of V8InspectorSession objects.
We use this in "dispatchProtocolMessage" for now.

The main reason is that blink is allowed to tear down V8 sessions on
the nested run-loop, while actual agent/session functions remain on
the stack. This CL attempts to fix this in a more general way than
patching adhocs UaF.

Note that its not clear if this fixes all instances where the session
dies on the nested run loop: There are other entry points into
inspector agent code that can then transition into JS and then run
a nested event loop, e.g. client events or interrupts.

Bug: 40071155
Change-Id: I48f599cfcda2cf8ed71e3b865a6d2e71daa59061
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6433946
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#99784}
---

diff --git a/include/v8-inspector.h b/include/v8-inspector.h
index 7236a37..0992cb1 100644
--- a/include/v8-inspector.h
+++ b/include/v8-inspector.h
@@ -408,12 +408,20 @@
   enum ClientTrustLevel { kUntrusted, kFullyTrusted };
   enum SessionPauseState { kWaitingForDebugger, kNotWaitingForDebugger };
   // TODO(chromium:1352175): remove default value once downstream change lands.
+  // Deprecated: Use `connectShared` instead.
   virtual std::unique_ptr<V8InspectorSession> connect(
       int contextGroupId, Channel*, StringView state,
       ClientTrustLevel client_trust_level,
-      SessionPauseState = kNotWaitingForDebugger) {
-    return nullptr;
-  }
+      SessionPauseState = kNotWaitingForDebugger) = 0;
+
+  // Same as `connect` but returns a std::shared_ptr instead.
+  // Embedders should not deconstruct V8 sessions while the nested run loop
+  // (V8InspectorClient::runMessageLoopOnPause) is running. To partially ensure
+  // this, we defer session deconstruction until no "dispatchProtocolMessages"
+  // remains on the stack.
+  virtual std::shared_ptr<V8InspectorSession> connectShared(
+      int contextGroupId, Channel* channel, StringView state,
+      ClientTrustLevel clientTrustLevel, SessionPauseState pauseState) = 0;
 
   // API methods.
   virtual std::unique_ptr<V8StackTrace> createStackTrace(
diff --git a/src/inspector/v8-inspector-impl.cc b/src/inspector/v8-inspector-impl.cc
index 8172e3a..f2091bd 100644
--- a/src/inspector/v8-inspector-impl.cc
+++ b/src/inspector/v8-inspector-impl.cc
@@ -147,6 +147,24 @@
 std::unique_ptr<V8InspectorSession> V8InspectorImpl::connect(
     int contextGroupId, V8Inspector::Channel* channel, StringView state,
     ClientTrustLevel client_trust_level, SessionPauseState pause_state) {
+  return std::unique_ptr<V8InspectorSession>(connectImpl(
+      contextGroupId, channel, state, client_trust_level, pause_state));
+}
+
+std::shared_ptr<V8InspectorSession> V8InspectorImpl::connectShared(
+    int contextGroupId, V8Inspector::Channel* channel, StringView state,
+    ClientTrustLevel client_trust_level, SessionPauseState pause_state) {
+  std::shared_ptr<V8InspectorSessionImpl> session(connectImpl(
+      contextGroupId, channel, state, client_trust_level, pause_state));
+  // TODO(crbug.com/40071155): Move to V8InspectorSessionImpl::create once the
+  // unique_ptr version is no longer required.
+  session->setWeakThis(session);
+  return session;
+}
+
+V8InspectorSessionImpl* V8InspectorImpl::connectImpl(
+    int contextGroupId, V8Inspector::Channel* channel, StringView state,
+    ClientTrustLevel client_trust_level, SessionPauseState pause_state) {
   int sessionId = ++m_lastSessionId;
   std::shared_ptr<V8DebuggerBarrier> debuggerBarrier;
   if (pause_state == kWaitingForDebugger) {
@@ -162,12 +180,11 @@
       m_debuggerBarriers.insert(it, {contextGroupId, debuggerBarrier});
     }
   }
-  std::unique_ptr<V8InspectorSessionImpl> session =
-      V8InspectorSessionImpl::create(this, contextGroupId, sessionId, channel,
-                                     state, client_trust_level,
-                                     std::move(debuggerBarrier));
-  m_sessions[contextGroupId][sessionId] = session.get();
-  return std::move(session);
+  V8InspectorSessionImpl* session = V8InspectorSessionImpl::create(
+      this, contextGroupId, sessionId, channel, state, client_trust_level,
+      std::move(debuggerBarrier));
+  m_sessions[contextGroupId][sessionId] = session;
+  return session;
 }
 
 void V8InspectorImpl::disconnect(V8InspectorSessionImpl* session) {
diff --git a/src/inspector/v8-inspector-impl.h b/src/inspector/v8-inspector-impl.h
index 514a341..d3c617b 100644
--- a/src/inspector/v8-inspector-impl.h
+++ b/src/inspector/v8-inspector-impl.h
@@ -85,6 +85,11 @@
                                               StringView state,
                                               ClientTrustLevel,
                                               SessionPauseState) override;
+  std::shared_ptr<V8InspectorSession> connectShared(int contextGroupId,
+                                                    V8Inspector::Channel*,
+                                                    StringView state,
+                                                    ClientTrustLevel,
+                                                    SessionPauseState) override;
   void contextCreated(const V8ContextInfo&) override;
   void contextDestroyed(v8::Local<v8::Context>) override;
   v8::MaybeLocal<v8::Context> contextById(int contextId) override;
@@ -159,6 +164,10 @@
   };
 
  private:
+  V8InspectorSessionImpl* connectImpl(int contextGroupId, V8Inspector::Channel*,
+                                      StringView state, ClientTrustLevel,
+                                      SessionPauseState);
+
   v8::Isolate* m_isolate;
   V8InspectorClient* m_client;
   std::unique_ptr<V8Debugger> m_debugger;
diff --git a/src/inspector/v8-inspector-session-impl.cc b/src/inspector/v8-inspector-session-impl.cc
index 916ddb5..0546ea0 100644
--- a/src/inspector/v8-inspector-session-impl.cc
+++ b/src/inspector/v8-inspector-session-impl.cc
@@ -90,14 +90,14 @@
   return InspectedContext::contextId(context);
 }
 
-std::unique_ptr<V8InspectorSessionImpl> V8InspectorSessionImpl::create(
+V8InspectorSessionImpl* V8InspectorSessionImpl::create(
     V8InspectorImpl* inspector, int contextGroupId, int sessionId,
     V8Inspector::Channel* channel, StringView state,
     V8Inspector::ClientTrustLevel clientTrustLevel,
     std::shared_ptr<V8DebuggerBarrier> debuggerBarrier) {
-  return std::unique_ptr<V8InspectorSessionImpl>(new V8InspectorSessionImpl(
-      inspector, contextGroupId, sessionId, channel, state, clientTrustLevel,
-      std::move(debuggerBarrier)));
+  return new V8InspectorSessionImpl(inspector, contextGroupId, sessionId,
+                                    channel, state, clientTrustLevel,
+                                    std::move(debuggerBarrier));
 }
 
 V8InspectorSessionImpl::V8InspectorSessionImpl(
@@ -359,6 +359,8 @@
 }
 
 void V8InspectorSessionImpl::dispatchProtocolMessage(StringView message) {
+  KeepSessionAliveScope keepAlive(*this);
+
   using v8_crdtp::span;
   using v8_crdtp::SpanFrom;
   span<uint8_t> cbor;
diff --git a/src/inspector/v8-inspector-session-impl.h b/src/inspector/v8-inspector-session-impl.h
index c1add39..630fc66 100644
--- a/src/inspector/v8-inspector-session-impl.h
+++ b/src/inspector/v8-inspector-session-impl.h
@@ -33,7 +33,7 @@
 class V8InspectorSessionImpl : public V8InspectorSession,
                                public protocol::FrontendChannel {
  public:
-  static std::unique_ptr<V8InspectorSessionImpl> create(
+  static V8InspectorSessionImpl* create(
       V8InspectorImpl*, int contextGroupId, int sessionId,
       V8Inspector::Channel*, StringView state,
       v8_inspector::V8Inspector::ClientTrustLevel,
@@ -108,6 +108,10 @@
     return m_clientTrustLevel;
   }
 
+  void setWeakThis(std::weak_ptr<V8InspectorSessionImpl> weakThis) {
+    m_weakThis = std::move(weakThis);
+  }
+
  private:
   V8InspectorSessionImpl(V8InspectorImpl*, int contextGroupId, int sessionId,
                          V8Inspector::Channel*, StringView state,
@@ -145,6 +149,20 @@
       m_inspectedObjects;
   bool use_binary_protocol_ = false;
   V8Inspector::ClientTrustLevel m_clientTrustLevel = V8Inspector::kUntrusted;
+
+  // On each call to "dispatchProtocolMessage", the session turns the weakThis
+  // reference into a strong one, so nested run loops are not able to fully
+  // deconstruct the V8 session until we return from the
+  // "dispatchProtocolMessage" call (i.e. no freed "this" remains on the stack).
+  class KeepSessionAliveScope {
+   public:
+    explicit KeepSessionAliveScope(const V8InspectorSessionImpl& session)
+        : m_this(session.m_weakThis.lock()) {}