Skip to content

inspector: fix crash when writing to closed inspector socket#64209

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Yayapapa:fix/inspector-socket-null-tcp
Jul 6, 2026
Merged

inspector: fix crash when writing to closed inspector socket#64209
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Yayapapa:fix/inspector-socket-null-tcp

Conversation

@Yayapapa

@Yayapapa Yayapapa commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix a null pointer dereference crash in the inspector socket when writing to a connection that has already received EOF or encountered a protocol error.

Root Cause

There are two crash paths, both caused by tcp_ being null when dereferenced:

Path 1: Async write after EOF

  1. uv_read_cb fires → WsHandler::OnEof()tcp_.reset() (sets tcp_ to nullptr)
  2. On a subsequent loop iteration, uv_async_t callback fires → RequestQueueData::DoDispatch()InspectorSocket::Write()WsHandler::Write()ProtocolHandler::WriteRaw() → dereferences null tcp_

Path 2: Write triggered during OnData parsing

  1. ParseWsFrames() encounters a compressed or error frame → calls OnEof()tcp_.reset()
  2. delegate()->OnWsFrame() callback triggers a write back to the client → WsHandler::Write()ProtocolHandler::WriteRaw() → dereferences null tcp_

Both execute on the same libuv event loop thread (no multi-threading), so mutexes cannot help.

Crash signature (Windows x64)

EXCEPTION_ACCESS_VIOLATION_READ, fault addr 0x148
TcpHolder::WriteRaw [inspector_socket.cc:733]
WsHandler::Write    [inspector_socket.cc:406]
InspectorSocket::Write [inspector_socket.cc:819]

0x148 is the offset of TcpHolder::handler_ — confirms this == nullptr (i.e., tcp_ was null when dereferenced).

Fix

  • WsHandler::OnData: stop the parsing loop when tcp_ becomes null mid-iteration
  • WsHandler::Write: early return when tcp_ is null (avoids unnecessary frame encoding)
  • ProtocolHandler::WriteRaw: defensive null guard covering all write paths

No resource leak: WriteRequest is only allocated inside TcpHolder::WriteRaw, which is never reached when tcp_ is null. The -1 return is consistent with uv_write failure semantics.

This is consistent with the existing pattern in WsHandler::Shutdown() which already checks if (tcp_) before use.

Fixes: #34833

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. labels Jun 30, 2026
@Yayapapa Yayapapa force-pushed the fix/inspector-socket-null-tcp branch from 6b24974 to fce2dc8 Compare June 30, 2026 07:28
Comment thread src/inspector_socket.cc
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 30, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Yayapapa Yayapapa requested a review from addaleax June 30, 2026 10:53
@Yayapapa Yayapapa force-pushed the fix/inspector-socket-null-tcp branch from 2a1112e to fce2dc8 Compare June 30, 2026 12:50
ProtocolHandler::WriteRaw() dereferences tcp_ without a null check.
When the remote end disconnects, OnEof() resets tcp_ to nullptr, but
queued messages from the uv_async callback can still trigger Write()
on the same event loop iteration, causing a null pointer dereference
crash (EXCEPTION_ACCESS_VIOLATION on Windows).

Additionally, ParseWsFrames() can call OnEof() internally (on
compressed or error frames), which resets tcp_ mid-loop in OnData().
If the delegate callback triggered by OnWsFrame() then calls Write(),
it would also hit the null tcp_ crash.

Add null guards in:
- WsHandler::OnData: stop parsing loop when tcp_ becomes null
- WsHandler::Write: early return before frame encoding
- ProtocolHandler::WriteRaw: defensive fallback for all write paths

Fixes: nodejs#34833
Signed-off-by: piaoyingmin <piaoyingmin@bytedance.com>
@Yayapapa Yayapapa force-pushed the fix/inspector-socket-null-tcp branch from fce2dc8 to 52c41aa Compare July 1, 2026 02:39
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 2, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Yayapapa

Yayapapa commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Hi, could someone help confirm whether the Jenkins CI failures (node-test-commit, node-test-commit-osx, node-test-commit-plinux-rhel8, node-test-pull-request) are flaky? The GitHub Actions CI is all green and the change is minimal (null checks only). Happy to rebase or re-trigger CI if needed. Thanks!

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2026
@nodejs-github-bot nodejs-github-bot merged commit 8a84e6b into nodejs:main Jul 6, 2026
78 of 79 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 8a84e6b

richardlau pushed a commit that referenced this pull request Jul 6, 2026
ProtocolHandler::WriteRaw() dereferences tcp_ without a null check.
When the remote end disconnects, OnEof() resets tcp_ to nullptr, but
queued messages from the uv_async callback can still trigger Write()
on the same event loop iteration, causing a null pointer dereference
crash (EXCEPTION_ACCESS_VIOLATION on Windows).

Additionally, ParseWsFrames() can call OnEof() internally (on
compressed or error frames), which resets tcp_ mid-loop in OnData().
If the delegate callback triggered by OnWsFrame() then calls Write(),
it would also hit the null tcp_ crash.

Add null guards in:
- WsHandler::OnData: stop parsing loop when tcp_ becomes null
- WsHandler::Write: early return before frame encoding
- ProtocolHandler::WriteRaw: defensive fallback for all write paths

Fixes: #34833
Signed-off-by: piaoyingmin <piaoyingmin@bytedance.com>
PR-URL: #64209
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in the inspector calls WriteRaw() on nullptr

7 participants