Skip to content

rpc : reject invalid top-level graph nodes in graph_compute - #25670

Open
liminfei-amd wants to merge 1 commit into
ggml-org:masterfrom
liminfei-amd:fix/25299-rpc-null-node
Open

rpc : reject invalid top-level graph nodes in graph_compute#25670
liminfei-amd wants to merge 1 commit into
ggml-org:masterfrom
liminfei-amd:fix/25299-rpc-null-node

Conversation

@liminfei-amd

@liminfei-amd liminfei-amd commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Overview

rpc_server::graph_compute() deserializes each top-level graph node id and
resolves it through create_node(). The old check only rejected a nullptr
result when the wire id was non-zero, treating a nullptr result for id 0
as "expected" and letting it into graph->nodes. id 0 is the wire sentinel
for "no tensor" and is valid for optional tensor sources (src[]/view_src),
but it is not a valid top-level graph node. The backend later dereferences
that node during graph planning, crashing the RPC server on a single crafted
GRAPH_COMPUTE message (device=0, n_nodes=1, node_id=0, n_tensors=0).

Fixes #25299.

Reject every failed top-level node reconstruction before calling
ggml_backend_graph_compute(), regardless of id.

Additional information

Verified with a localhost-only RPC client sending the malformed message above
against a CPU-only build (no ROCm/HIP): before the fix the server exits with
SIGSEGV; after the fix the malformed connection is closed and a subsequent
client can still connect and complete HELLO normally.

A regression test (test-rpc-invalid-graph-node) is included; it starts the
RPC server, sends the crafted message, confirms the connection is dropped, and
verifies the server stays alive for new connections.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES - an AI assistant helped locate the root cause and drafted this fix; I reviewed the surrounding create_node/graph_compute code, reproduced the crash and the fix's effect with a local RPC client, and can explain every line of the change.

@github-actions github-actions Bot added examples ggml changes relating to the ggml tensor library for machine learning labels Jul 14, 2026
@ruixiang63 ruixiang63 closed this Jul 15, 2026
@ruixiang63 ruixiang63 reopened this Jul 15, 2026
@liminfei-amd
liminfei-amd marked this pull request as ready for review July 17, 2026 07:04
@liminfei-amd
liminfei-amd requested a review from a team as a code owner July 17, 2026 07:04
@liminfei-amd
liminfei-amd force-pushed the fix/25299-rpc-null-node branch from 91b62a7 to 3900550 Compare August 31, 2026 06:33
id 0 is a valid "no tensor" sentinel for optional sources, not for a
top-level graph node. Reject any failed node reconstruction regardless
of id, before running the graph.

Fixes ggml-org#25299
Reported-by: professor-moody

Assisted-by: GitHub Copilot
@liminfei-amd
liminfei-amd force-pushed the fix/25299-rpc-null-node branch from 3900550 to 181b199 Compare August 31, 2026 06:55
@liminfei-amd

Copy link
Copy Markdown
Contributor Author

@ruixiang63 you reopened this PR back in July — would you be able to take a look,
or point it at whoever should review it?

Rebased onto current master. The branch had gone stale and was conflicting. The
only conflict was in tools/rpc/CMakeLists.txt, which gained the
test-rpc-multi-server block in #26500. Both tests are kept. Mine is now gated
on LLAMA_BUILD_TESTS — it was on the generic BUILD_TESTING before, which this
project does not use, so it would not have registered in a normal test build.

I also removed a branch that my own change had made dead: once the guard returns
on nullptr, the following if (graph->nodes[i] != nullptr) is always true.
Removing it is safe because create_node() returns non-null only after
tensor_ptrs.find(id) succeeds, so the tensor_ptrs.at(id) below it cannot
throw.

The crash still reproduces on current master. Same test harness on both sides,
with only the guard change differing:

master this PR
test-rpc-invalid-graph-node FAILED passed
server after a malformed GRAPH_COMPUTE dies, SIGSEGV logs the rejection and keeps serving

test-rpc-multi-server also passes on this branch, which covers the normal graph
path through the code I touched. Both builds are CPU-only.

One thing I cannot do from here: the workflow runs on this head are all
action_required, so CI has not started. If you are able to approve them, that
would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

examples ggml changes relating to the ggml tensor library for machine learning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote unauthenticated NULL-pointer dereference in ggml-rpc graph_compute() via a node id of 0 (ggml_graph_plan/ggml_is_empty)

2 participants