Reviewed by ByteBell

ggml-org/llama.cpp

LLM inference in C/C++

#29915 ggml-rpc: validate PAD_REFLECT_1D parameters

Judged at indexed commitedd6e2bbda
Base1537a0a8b2
Head004d67ed03

5 Oct 2026 at 23:20 UTC

This PR adds validation for PAD_REFLECT_1D over RPC but leaves gaps that allow OOB reads and backend aborts.

The change needs tighter validation matching the builder's assertions, plus broader test coverage.

Detailed review

1. Missing p0 < ne00 and p1 < ne00 checks in RPC validation (major security)

The RPC validation in ggml/src/ggml-rpc/ggml-rpc.cpp:1791–1798 rejects p0<0, p1<0, and ne[0] < ne00+p0+p1, but does not check p0 < ne00 or p1 < ne00:

// ggml/src/ggml-rpc/ggml-rpc.cpp:1794–1798

The normal producer ggml_pad_reflect_1d asserts p0 < a->ne[0] and p1 < a->ne[0] at ggml/src/ggml.c:5304–5308. Without these checks, the CPU kernel's reflect loop reads left[i0] for i0=1..p0 (positions p0+1 to 2*p0) at ggml/src/ggml-cpu/ops.cpp:8533–8539. If p0 >= ne00, position 2*p0 exceeds the copied region and can read past the buffer end — an OOB read. Add p0 < ne00 and p1 < ne00 checks to the RPC validation.

2. Lower-bound-only ne[0] check allows backend aborts (minor bug)

The RPC check uses < (lower bound only):

// ggml/src/ggml-rpc/ggml-rpc.cpp:1794–1795
// node->ne[0] < node->src[0]->ne[0] + p0 + p1

This allows ne[0] > ne00+p0+p1 through. The CUDA kernel at ggml/src/ggml-cuda/pad_reflect_1d.cu:78 and the SYCL kernel at ggml/src/ggml-sycl/pad_reflect_1d.cpp:70 both assert ne0 == ne00 + p0 + p1 — equality, not just a lower bound. A graph with ne0 > ne00+p0+p1 passes RPC validation but aborts on those backends. Change the RPC check to require equality, or at least an upper bound, matching the CUDA/SYCL assertion.

3. Null src[0] skips validation (minor bug)

The validation condition at ggml/src/ggml-rpc/ggml-rpc.cpp:1787–1789 causes continue when node->src[0] == nullptr, skipping validation entirely. A PAD_REFLECT_1D node with src[0]=0 from the wire passes create_node and reaches the kernel unvalidated. The CPU kernel at ggml/src/ggml-cpu/ops.cpp:8516–8518 does src0 = dst->src[0] then GGML_ASSERT(src0->type == GGML_TYPE_F32) — a null src0 causes a null dereference before any assertion catches it. Reject null src[0] instead of skipping validation.

4. Kernel assertions in ops.cpp omit reflect-read and dimension checks (minor security)

The added assertions at ggml/src/ggml-cpu/ops.cpp:8530–8532 check only p0>=0, p1>=0, and ne0>=ne00+p0+p1:

// ggml/src/ggml-cpu/ops.cpp:8530–8532

The builder at ggml/src/ggml.c:5307–5308 asserts p0 < a->ne[0] and p1 < a->ne[0], preventing reflect reads past copied source data. The builder at ggml/src/ggml.c:5313–5317 sets dst ne[1..3] equal to source ne[1..3], ensuring loop bounds match source dimensions. The reflect loop at ops.cpp:8533–8539 iterates i1 over ne1 (dst) but reads src0 at i1*nb01; if ne1 > ne01, the source read is OOB. Add p0 < ne00, p1 < ne00, and ne[1..3] == src[0]->ne[1..3] assertions to the kernel.

5. Test coverage and documentation (minor tests)

  • The test at tests/test-rpc-malformed-graph.cpp:28–36 constructs exactly one malformed case: dst=8 floats, src=64 floats, p0=4, p1=0. It only exercises the ne[0] insufficient condition. No valid PAD_REFLECT_1D case is tested, so an over-strict regression in the RPC validation would not be caught.
  • No test covers p0>=ne00 with large dst — the case that passes F2's check but reads past src, which is the major security gap.
  • No test covers negative p0/p1, ne[1..3] mismatch, or null src[0].
  • The script tests/test-rpc-malformed-graph.sh:33–48 runs only the malformed client and checks for rejection plus server liveness; it never sends a valid PAD_REFLECT_1D graph to confirm legitimate inputs are accepted.
  • Add a valid case and additional malformed cases covering each validation branch.
0blocker
1major
4minor
5/5hunks reviewed
9base files read
14m 1stime
$1.68cost

3.42M input and 158k output tokens.

5 findings — 5 of 5 files reviewed.

Findings

majorSecurityggml/src/ggml-rpc/ggml-rpc.cpp:1794–1798

RPC validation omits p0 < ne00 and p1 < ne00 checks that the normal producer enforces, leaving OOB reads in the CPU kernel's reflect loops.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1791-1798 this change — The new RPC check rejects p0<0, p1<0, and ne[0]<ne00+p0+p1, but does NOT check p0<ne00 or p1<ne00.
  • ggml/src/ggml.c:5304-5308 base — The normal producer ggml_pad_reflect_1d asserts p0 < a->ne[0] and p1 < a->ne[0] — padding must be less than input length. The RPC check omits this.
  • ggml/src/ggml-cpu/ops.cpp:8533-8539 base — CPU kernel reflect loop reads left[i0] for i0=1..p0 (positions p0+1 to 2*p0). If p0>=ne00, position 2*p0 exceeds the copied region and can read past the buffer end — OOB read.
minorBugggml/src/ggml-rpc/ggml-rpc.cpp:1795

RPC check allows ne[0] > ne00+p0+p1 but CUDA/SYCL kernels assert ne0 == ne00+p0+p1, so a graph passing RPC validation aborts on those backends.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1794-1795 this change — RPC check uses '<' (lower bound only): node->ne[0] < node->src[0]->ne[0] + p0 + p1. This allows ne[0] > ne00+p0+p1 through.
  • ggml/src/ggml-cuda/pad_reflect_1d.cu:78-78 base — CUDA kernel asserts GGML_ASSERT(ne0 == ne00 + p0 + p1) — equality, not just lower bound. A graph with ne0 > ne00+p0+p1 passes RPC check but aborts here.
  • ggml/src/ggml-sycl/pad_reflect_1d.cpp:70-70 base — SYCL kernel asserts GGML_ASSERT(ne0 == ne00 + p0 + p1) — same equality check. Same abort on non-CPU backend.
minorBugggml/src/ggml-rpc/ggml-rpc.cpp:1787

Validation skips PAD_REFLECT_1D nodes with null src[0] instead of rejecting them, allowing a null dereference to reach the kernel.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1787-1789 this change — The condition 'node->src[0] == nullptr' causes continue, skipping validation. A PAD_REFLECT_1D node with src[0]=0 from the wire passes create_node and reaches the kernel unvalidated.
  • ggml/src/ggml-cpu/ops.cpp:8516-8518 base — CPU kernel does src0 = dst->src[0] then GGML_ASSERT(src0->type == GGML_TYPE_F32) — null src0 causes null dereference before any assertion catches it.
minorSecurityggml/src/ggml-cpu/ops.cpp:8531

Kernel assertions omit p0<ne00, p1<ne00, and ne[1..3]==src[0]->ne[1..3] checks the builder enforces; reflect logic can OOB-read when p0>=ne00 or outer dims mismatch.

  • ggml/src/ggml.c:5307-5308 base — Builder asserts p0 < a->ne[0] and p1 < a->ne[0], preventing reflect reads past copied source data.
  • ggml/src/ggml.c:5313-5317 base — Builder sets dst ne[1..3] equal to source ne[1..3], ensuring loop bounds match source dimensions.
  • ggml/src/ggml-cpu/ops.cpp:8533-8539 base — Reflect loop reads left[i0] for i0=1..p0; if p0>=ne00, reads past the ne00 copied floats. Loop iterates i1 over ne1 (dst) but reads src0 at i1*nb01; if ne1>ne01, source read is OOB.
  • ggml/src/ggml-cpu/ops.cpp:8530-8532 this change — Added assertions check p0>=0, p1>=0, and ne0>=ne00+p0+p1 only — not p0<ne00, p1<ne00, or ne[1..3] equality.
minorTestsggml/src/ggml-rpc/ggml-rpc.cpp:1794

Test only covers one malformed case (dst<src+p0+p1); no test for valid PAD_REFLECT_1D over RPC, negative p0/p1, p0>=ne00, or ne[1..3] mismatch.

  • tests/test-rpc-malformed-graph.cpp:28-36 this change — Test constructs exactly one malformed case: dst=8 floats, src=64 floats, p0=4, p1=0. No valid case or other invalid parameter combinations are tested.
  • ggml/src/ggml-rpc/ggml-rpc.cpp:1794-1798 this change — The validation checks four distinct conditions (p0<0, p1<0, ne[0] insufficient, ne[1..3] mismatch) but the test only exercises the ne[0] condition.
  • tests/test-rpc-malformed-graph.sh:33-48 this change — The script runs only the malformed client and checks for rejection + server liveness. It never sends a valid PAD_REFLECT_1D graph to confirm the new validation doesn't reject legitimate inputs.
  • tests/test-rpc-malformed-graph.cpp:1-64 this change — Test only constructs a malformed graph (dst=8, src=64, p0=4) and checks rejection; no valid PAD_REFLECT_1D case is tested.
  • ggml/src/ggml.c:5299-5326 base — Public builder ggml_pad_reflect_1d validates p0<p0<ne[0], p1<ne[0], contiguous, F32; F2's check is weaker, so an over-strict F2 regression would not be caught.
  • ggml/src/ggml-rpc/ggml-rpc.cpp:1791-1801 this change — F2 checks p0>=0, p1>=0, ne[0]>=ne00+p0+p1 but does NOT check p0<ne00 or p1<ne00 (reflect read range).
  • ggml/src/ggml.c:5307-5308 base — Public builder asserts p0 < a->ne[0] and p1 < a->ne[0] — the reflect-read bound that F2 omits.
  • tests/test-rpc-malformed-graph.cpp:30-36 this change — Test only covers write-OOB (p0=4, dst=8, src=64); no case where p0>=ne00 with large dst passes F2 but reads past src.

How the analysis was done

Changes, file by file

ggml/src/ggml-cpu/ops.cpp+3 −0 · modified · 1 finding

Analysis of this file

I have filed my finding and read all the evidence I need. Let me finalize. Closing lines — F1 ggml/src/ggml-cpu/ops.cpp review: What I checked: - bug: Read the full base function ggml_compute_forward_pad_reflect_1d (lines 8512–8543). The two added assertions are correctly placed after GGML_TENSOR_UNARY_OP_LOCALS (which defines ne0/ne00 via GGML_TENSOR_LOCALS) and before the write loop. The assertions prevent the OOB *write* (the primary vulnerability): ne0 >= ne00 + p0 + p1 ensures ggml_vec_cpy_f32(ne00, left, ...) writes within bounds, and left[-i0]/right[i0] writes stay in bounds. No bug in what the assertions check. - security: Filed one finding (minor): the kernel assertions are incomplete vs the builder ggml_pad_reflect_1d (ggml.c:5304–5317). They omit p0 < ne00, p1 < ne00, and ne[1..3] == src[0]->ne[1..3]. Without p0 < ne00, the reflect loop left[-i0] = left[i0] for i0 = 1..p0 can read left[p0] at offset 2*p0 from the row start — if 2*p0 >= ne0 (possible when p0 >= ne00 and ne00 is small), this is an OOB read from the destination buffer. Without ne[1..3] checks, the loop iterates i1 over ne1 (destination) but reads src0 at i1*nb01 — if ne1 > ne01, source read is OOB. The builder prevents both; the F2 RPC check prevents the dimension mismatch but also omits p0 < ne00. These are OOB reads, not writes — less severe than the original vulnerability, and defense-in-depth only (F2 is the primary gate), hence minor. - convention: The function already uses GGML_ASSERT for type checks (lines 8518–8519). The new assertions follow the same idiom. No convention issue. - duplicate: No — these assertions did not exist in the base function. - dead-code/stale-text: None introduced. No comments or names made stale. - performance: Two integer comparisons per kernel invocation — negligible. - compatibility: No wire format, schema, or API change. Valid graphs pass unchanged (builder enforces a stricter invariant). - breaks-consumer: No callers invalidated — the assertion only fires on invalid inputs that would corrupt memory. Disposition of analysis concerns: - NEEDED: Confirmed — base function had zero validation of p0/p1. PR addresses it. No finding. - REFLECTS: Filed — kernel assertion is weaker than builder (missing p0 < ne00, p1 < ne00, ne[1..3] checks). - BREAKING: Refuted — builder (ggml.c:5313–5317) sets ne[1..3] equal and ne[0] = ne00 + p0 + p1; valid graphs pass both assertions. - TESTS: Open — the kernel assertion is not exercised in isolation (F2 rejects before reaching the kernel; the builder prevents invalid local graphs). A unit test bypassing the builder would be needed to cover it. Minor gap, not filed as a separate finding since F1 is defense-in-depth. - RELATED CHANGES: Not F1-specific — other ops with unchecked op_params are a broader concern for F2's session. - COST: Refuted — two comparisons, no allocations. - DEAD CODE/STALE TEXT: Refuted — none introduced. - SECURITY/COMPATIBILITY: Confirmed — fixes OOB write; no wire format change. What I did not get to: Whether other backend implementations (CUDA, Metal, Vulkan, SYCL) of pad_reflect_1d need the same kernel-level assertions — that is outside F1's scope and belongs to those files' sessions.
F1.H11 finding
void ggml_compute_forward_pad_reflect_1d(
85278527
85288528 GGML_TENSOR_UNARY_OP_LOCALS
85298529
8530+ GGML_ASSERT(p0 >= 0 && p1 >= 0);
8531+ GGML_ASSERT(ne0 >= ne00 + p0 + p1);
8532+
85308533 for (int64_t i3 = 0; i3 < ne3; i3++) {
85318534 for (int64_t i2 = 0; i2 < ne2; i2++) {
85328535 for (int64_t i1 = ith; i1 < ne1; i1 += nth) {
minorSecurityggml/src/ggml-cpu/ops.cpp:8531

Kernel assertions omit p0<ne00, p1<ne00, and ne[1..3]==src[0]->ne[1..3] checks the builder enforces; reflect logic can OOB-read when p0>=ne00 or outer dims mismatch.

  • ggml/src/ggml.c:5307-5308 base — Builder asserts p0 < a->ne[0] and p1 < a->ne[0], preventing reflect reads past copied source data.
  • ggml/src/ggml.c:5313-5317 base — Builder sets dst ne[1..3] equal to source ne[1..3], ensuring loop bounds match source dimensions.
  • ggml/src/ggml-cpu/ops.cpp:8533-8539 base — Reflect loop reads left[i0] for i0=1..p0; if p0>=ne00, reads past the ne00 copied floats. Loop iterates i1 over ne1 (dst) but reads src0 at i1*nb01; if ne1>ne01, source read is OOB.
  • ggml/src/ggml-cpu/ops.cpp:8530-8532 this change — Added assertions check p0>=0, p1>=0, and ne0>=ne00+p0+p1 only — not p0<ne00, p1<ne00, or ne[1..3] equality.
ggml/src/ggml-rpc/ggml-rpc.cpp+23 −0 · modified · 4 findings

Analysis of this file

All findings I have evidence for are already flagged. Let me verify I haven't missed any by reviewing my findings against the analysis concerns: Findings flagged (4): 1. Security (major) — RPC check omits p0 < ne00 / p1 < ne00, leaving OOB reads in CPU kernel reflect loops 2. Bug (minor) — Null src[0] skipped instead of rejected, allowing null dereference to reach kernel 3. Tests (minor) — Only one malformed case tested; no valid case, negative p0/p1, p0>=ne00, or ne[1..3] mismatch tested 4. Bug (minor) — RPC check uses < but CUDA/SYCL assert ==, so graphs with ne[0] > ne00+p0+p1 pass RPC but abort on GPU backends Concerns disposed: - NEEDED: Refuted — deserialize_tensor copies op_params verbatim from untrusted network input (line 1420), no validation exists between boundary and kernel. Fix is needed. - BREAKING (kernel assertions break local callers): Refuted — normal producer ggml_pad_reflect_1d (ggml.c:5304-5308) already asserts p0>=0, p1>=0, p0<ne[0], p1<ne[0]; new kernel assertions are weaker, can't break anything the producer didn't already guard. - COST (O(n_nodes) scan): Refuted — negligible per-node comparisons, no allocations. - DEAD CODE AND STALE TEXT: Refuted — comment accurately describes the vulnerability; no dead code added. - COMPATIBILITY: Refuted — no wire format, schema, or API change; server-side validation only. - SECURITY (other ops' op_params unvalidated): Refuted for this PR — scoped to PAD_REFLECT_1D per issue #29914; other ops are a separate concern for a separate PR (#26933 covers SET_ROWS/GET_ROWS). - SECURITY (ne[], nb[], type, use_count unvalidated): Refuted for this PR — same scoping rationale; broader tensor validation is a separate effort. - graph_recompute path: Refuted — re-executes a graph already validated in graph_compute, so the same checks apply. What I checked: bug (null deref, missing checks, inequality mismatch), security (incomplete validation vs. producer invariants), tests (coverage gaps), convention (comparison with CUDA/SYCL/CANN kernel assertions), dead-code, stale-text, performance, compatibility, breaks-consumer. What I did not get to: F1 (ops.cpp kernel assertions), F4 (test shell script), and F5 (CMakeLists) are reviewed in their own sessions. I did not verify whether the Vulkan pad_reflect_1d kernel has its own validation gaps (it uses a compute shader, not the same assertion pattern). I did not check whether ggml-virtgpu's copy of the RPC protocol needs the same validation.
F2.H14 findings
bool rpc_server::graph_compute(const std::vector<uint8_t> & input) {
17781778 graph->use_counts[hash_pos] = tensor_ptrs.at(id)->use_count;
17791779 }
17801780 }
1781+ // Reject remote graphs whose PAD_REFLECT_1D node would make the CPU kernel write
1782+ // outside of the destination tensor (op_params of a remote node are not validated
1783+ // anywhere else, so p0/p1 can be arbitrary, including negative values).
1784+ for (uint32_t i = 0; i < n_nodes; i++) {
1785+ const struct ggml_tensor * node = graph->nodes[i];
1786+
1787+ if (node == nullptr || node->op != GGML_OP_PAD_REFLECT_1D || node->src[0] == nullptr) {
1788+ continue;
1789+ }
1790+
1791+ const int32_t p0 = ((const int32_t *) node->op_params)[0];
1792+ const int32_t p1 = ((const int32_t *) node->op_params)[1];
1793+
1794+ if (p0 < 0 || p1 < 0 ||
1795+ node->ne[0] < node->src[0]->ne[0] + p0 + p1 ||
1796+ node->ne[1] != node->src[0]->ne[1] ||
1797+ node->ne[2] != node->src[0]->ne[2] ||
1798+ node->ne[3] != node->src[0]->ne[3]) {
1799+ GGML_LOG_ERROR("[%s] malformed PAD_REFLECT_1D graph detected\n", __func__);
1800+ return false;
1801+ }
1802+ }
1803+
17811804 ggml_status status = ggml_backend_graph_compute(backends[device], graph);
17821805 GGML_ASSERT(status == GGML_STATUS_SUCCESS && "Unsuccessful graph computations are not supported with RPC");
17831806 stored_graphs[device].graph = graph;
majorSecurityggml/src/ggml-rpc/ggml-rpc.cpp:1794–1798

RPC validation omits p0 < ne00 and p1 < ne00 checks that the normal producer enforces, leaving OOB reads in the CPU kernel's reflect loops.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1791-1798 this change — The new RPC check rejects p0<0, p1<0, and ne[0]<ne00+p0+p1, but does NOT check p0<ne00 or p1<ne00.
  • ggml/src/ggml.c:5304-5308 base — The normal producer ggml_pad_reflect_1d asserts p0 < a->ne[0] and p1 < a->ne[0] — padding must be less than input length. The RPC check omits this.
  • ggml/src/ggml-cpu/ops.cpp:8533-8539 base — CPU kernel reflect loop reads left[i0] for i0=1..p0 (positions p0+1 to 2*p0). If p0>=ne00, position 2*p0 exceeds the copied region and can read past the buffer end — OOB read.
minorBugggml/src/ggml-rpc/ggml-rpc.cpp:1795

RPC check allows ne[0] > ne00+p0+p1 but CUDA/SYCL kernels assert ne0 == ne00+p0+p1, so a graph passing RPC validation aborts on those backends.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1794-1795 this change — RPC check uses '<' (lower bound only): node->ne[0] < node->src[0]->ne[0] + p0 + p1. This allows ne[0] > ne00+p0+p1 through.
  • ggml/src/ggml-cuda/pad_reflect_1d.cu:78-78 base — CUDA kernel asserts GGML_ASSERT(ne0 == ne00 + p0 + p1) — equality, not just lower bound. A graph with ne0 > ne00+p0+p1 passes RPC check but aborts here.
  • ggml/src/ggml-sycl/pad_reflect_1d.cpp:70-70 base — SYCL kernel asserts GGML_ASSERT(ne0 == ne00 + p0 + p1) — same equality check. Same abort on non-CPU backend.
minorBugggml/src/ggml-rpc/ggml-rpc.cpp:1787

Validation skips PAD_REFLECT_1D nodes with null src[0] instead of rejecting them, allowing a null dereference to reach the kernel.

  • ggml/src/ggml-rpc/ggml-rpc.cpp:1787-1789 this change — The condition 'node->src[0] == nullptr' causes continue, skipping validation. A PAD_REFLECT_1D node with src[0]=0 from the wire passes create_node and reaches the kernel unvalidated.
  • ggml/src/ggml-cpu/ops.cpp:8516-8518 base — CPU kernel does src0 = dst->src[0] then GGML_ASSERT(src0->type == GGML_TYPE_F32) — null src0 causes null dereference before any assertion catches it.
minorTestsggml/src/ggml-rpc/ggml-rpc.cpp:1794

Test only covers one malformed case (dst<src+p0+p1); no test for valid PAD_REFLECT_1D over RPC, negative p0/p1, p0>=ne00, or ne[1..3] mismatch.

  • tests/test-rpc-malformed-graph.cpp:28-36 this change — Test constructs exactly one malformed case: dst=8 floats, src=64 floats, p0=4, p1=0. No valid case or other invalid parameter combinations are tested.
  • ggml/src/ggml-rpc/ggml-rpc.cpp:1794-1798 this change — The validation checks four distinct conditions (p0<0, p1<0, ne[0] insufficient, ne[1..3] mismatch) but the test only exercises the ne[0] condition.
  • tests/test-rpc-malformed-graph.sh:33-48 this change — The script runs only the malformed client and checks for rejection + server liveness. It never sends a valid PAD_REFLECT_1D graph to confirm the new validation doesn't reject legitimate inputs.
  • tests/test-rpc-malformed-graph.cpp:1-64 this change — Test only constructs a malformed graph (dst=8, src=64, p0=4) and checks rejection; no valid PAD_REFLECT_1D case is tested.
  • ggml/src/ggml.c:5299-5326 base — Public builder ggml_pad_reflect_1d validates p0<p0<ne[0], p1<ne[0], contiguous, F32; F2's check is weaker, so an over-strict F2 regression would not be caught.
  • ggml/src/ggml-rpc/ggml-rpc.cpp:1791-1801 this change — F2 checks p0>=0, p1>=0, ne[0]>=ne00+p0+p1 but does NOT check p0<ne00 or p1<ne00 (reflect read range).
  • ggml/src/ggml.c:5307-5308 base — Public builder asserts p0 < a->ne[0] and p1 < a->ne[0] — the reflect-read bound that F2 omits.
  • tests/test-rpc-malformed-graph.cpp:30-36 this change — Test only covers write-OOB (p0=4, dst=8, src=64); no case where p0>=ne00 with large dst passes F2 but reads past src.
tests/test-rpc-malformed-graph.cpp+64 −0 · added
F3.H1
1+// Regression test: a remote graph whose PAD_REFLECT_1D node has op_params that would make
2+// the CPU kernel write outside the destination tensor must be rejected by the server
3+// instead of corrupting its heap.
4+//
5+// Before the fix the server aborts ("double free or corruption") under the same input.
6+#include "ggml-backend.h"
7+#include "ggml-rpc.h"
8+#include "ggml.h"
9+
10+#include <csignal>
11+#include <cstdio>
12+#include <unistd.h>
13+
14+int main(int argc, char ** argv) {
15+ signal(SIGPIPE, SIG_IGN);
16+
17+ GGML_ASSERT(argc == 2);
18+ const char * endpoint = argv[1];
19+
20+ ggml_init_params params = {
21+ /* .mem_size = */ 16u*1024u*1024u,
22+ /* .mem_buffer = */ nullptr,
23+ /* .no_alloc = */ true,
24+ };
25+ ggml_context * ctx_dst = ggml_init(params);
26+ ggml_context * ctx_src = ggml_init(params);
27+
28+ // destination: 8 floats (32 bytes), source: 64 floats; p0 = 4 puts the 64-float copy
29+ // past the end of the destination allocation.
30+ ggml_tensor * dst = ggml_new_tensor_1d(ctx_dst, GGML_TYPE_F32, 8);
31+ ggml_tensor * src0 = ggml_new_tensor_1d(ctx_src, GGML_TYPE_F32, 64);
32+
33+ dst->op = GGML_OP_PAD_REFLECT_1D;
34+ dst->src[0] = src0;
35+ ((int32_t *) dst->op_params)[0] = 4;
36+ ((int32_t *) dst->op_params)[1] = 0;
37+
38+ ggml_cgraph * graph = ggml_new_graph(ctx_src);
39+ ggml_build_forward_expand(graph, dst);
40+
41+ ggml_backend_t backend = ggml_backend_rpc_init(endpoint, 0);
42+ GGML_ASSERT(backend != nullptr);
43+ ggml_backend_buffer_t buf_dst = ggml_backend_alloc_ctx_tensors(ctx_dst, backend);
44+ ggml_backend_buffer_t buf_src = ggml_backend_alloc_ctx_tensors(ctx_src, backend);
45+ GGML_ASSERT(buf_dst != nullptr);
46+ GGML_ASSERT(buf_src != nullptr);
47+
48+ float values[64];
49+ for (size_t i = 0; i < 64; ++i) {
50+ values[i] = (float) i;
51+ }
52+ ggml_backend_tensor_set(src0, values, 0, sizeof(values));
53+
54+ // server must refuse the graph; the connection may be torn down, so ignore the result
55+ (void) ggml_backend_graph_compute(backend, graph);
56+ ggml_backend_synchronize(backend);
57+
58+ // The server has already closed the connection after refusing the graph, so any
59+ // further RPC call (including the remote buffer frees) would abort the client.
60+ // The server owns no state for this connection anymore; just exit.
61+ (void) buf_dst;
62+ (void) buf_src;
63+ _exit(0);
64+}
tests/test-rpc-malformed-graph.sh+48 −0 · added

Analysis of this file

Now let me verify there are no other issues I should check. Let me look at the set -euo pipefail interaction with wait_for_port more carefully. The script uses set -e (from set -euo pipefail). On line 31, wait_for_port "$port" is called. If it returns 1 (timeout), set -e would cause the script to exit with failure — but the trap cleanup EXIT would still run, killing the server. This is correct behavior: if the server doesn't start, the test should fail. However, there's a subtle issue: wait_for_port uses for _ in {1..600} with sleep 0.05 — that's 30 seconds max. If the server fails to start, the script exits via set -e, the trap kills the (possibly already dead) server, and the test fails. This is fine. Let me also check whether the kill "${pid:-}" in cleanup could cause issues. If pid is never set (e.g., server launch fails before pid=$!), ${pid:-} expands to empty string, and kill "" 2>/dev/null || true — this would actually try to kill with empty arg, which kill treats as a usage error, but 2>/dev/null || true swallows it. This matches the existing script's convention exactly. One more thing to verify: the CMake integration. F5.H1 shows the test is added under the same if (LLAMA_BUILD_TESTS AND UNIX AND NOT GGML_BACKEND_DL) guard as the existing test. The COMMAND bash ${PROJECT_SOURCE_DIR}/tests/test-rpc-malformed-graph.sh follows the same pattern. Good. Let me check if there's a bash dependency issue — the existing test also uses bash, so this is consistent. I've now covered all the finding kinds: - bug: No bugs found in the script logic - breaks-consumer: N/A (test file, no consumers) - duplicate: The script is modeled on test-rpc-multi-server.sh but is not a duplicate — different test purpose - convention: Follows existing convention closely - dead-code: No dead code - stale-text: Comments are accurate - performance: N/A for a test script - security: The test correctly verifies the security fix - compatibility: CMake integration is correct - tests: Filed the missing positive test finding I'm satisfied I've covered everything. Let me write my closing lines. --- Closing lines — F4 tests/test-rpc-malformed-graph.sh What I checked: - bug: Reviewed all 48 lines for logic errors — server launch, port wait, client invocation, log grep, liveness check, cleanup trap. No bugs found. The || true on the client call (line 33) is correct since the server tears down the connection after rejecting the graph. set -euo pipefail interacts correctly with wait_for_port (failure exits via set -e, trap fires). - convention: Compared against the existing tests/test-rpc-multi-server.sh — same shebang, set -euo pipefail, argument structure, wait_for_port function, cleanup/trap EXIT pattern, server launch flags, and CMake wiring (F5). Port base shifted from 40000 to 42000 to avoid collision. Consistent. - duplicate: Not a duplicate — different test purpose from test-rpc-multi-server.sh. - dead-code / stale-text: No dead code. Comments on lines 36 and 43 are accurate. The sleep 1 on line 34 is a pragmatic flush wait, not dead code. - security: The test correctly verifies the security fix — it confirms the server rejects the malformed graph (grep) and stays alive (kill -0). The log message in F4 line 37 ("malformed PAD_REFLECT_1D graph detected") matches the GGML_LOG_ERROR format string in F2.H1 line 1799. - compatibility: CMake integration (F5.H1) uses the same guard (LLAMA_BUILD_TESTS AND UNIX AND NOT GGML_BACKEND_DL) and same bash invocation pattern as the existing test. - performance: N/A for a test script. Disposition of analysis concerns: - NEEDED: Confirmed — the test exercises the exact attack vector described in the PR. - TESTS (missing positive test): FILED as minor — no test verifies a valid PAD_REFLECT_1D graph is still accepted after the new validation. - TESTS (kernel GGML_ASSERT not tested): Refuted — the kernel assertion would abort the process, so it cannot be tested in the normal framework; the RPC rejection path is the testable surface, and it is tested. - TESTS (mismatched dimensions 1–3): Refuted — the malformed test (F3) uses a 1-D tensor where dimensions 1–3 are all 1 for both dst and src, so they match; the test targets the ne[0] overflow case, which is the actual vulnerability. - SECURITY AND COMPATIBILITY: The test validates the RPC boundary rejection; no wire format or API change to test. What I did not get to: Nothing remained unexamined for this file.
F4.H1
1+#!/usr/bin/env bash
2+set -euo pipefail
3+
4+server=$1
5+client=$2
6+port=$((42000 + $$ % 10000))
7+endpoint="127.0.0.1:${port}"
8+test_dir=$(mktemp -d)
9+
10+cleanup() {
11+ kill "${pid:-}" 2>/dev/null || true
12+ rm -rf "$test_dir"
13+}
14+trap cleanup EXIT
15+
16+wait_for_port() {
17+ local p=$1
18+ for _ in {1..600}; do
19+ if (exec 3<>"/dev/tcp/127.0.0.1/$p") 2>/dev/null; then
20+ exec 3>&-
21+ exec 3<&-
22+ return 0
23+ fi
24+ sleep 0.05
25+ done
26+ return 1
27+}
28+
29+"$server" --device CPU --host 127.0.0.1 --port "$port" >"$test_dir/server.log" 2>&1 &
30+pid=$!
31+wait_for_port "$port"
32+
33+"$client" "$endpoint" || true
34+sleep 1
35+
36+# the server must have refused the graph ...
37+if ! grep -q "malformed PAD_REFLECT_1D graph detected" "$test_dir/server.log"; then
38+ echo "server did not report the malformed graph:"
39+ cat "$test_dir/server.log"
40+ exit 1
41+fi
42+
43+# ... and it must still be running
44+if ! kill -0 "$pid" 2>/dev/null; then
45+ echo "server died on the malformed graph:"
46+ cat "$test_dir/server.log"
47+ exit 1
48+fi
tools/rpc/CMakeLists.txt+10 −0 · modified

Analysis of this file

Now I have the full picture. Let me analyze what I've read: Base CMakeLists.txt (lines 1-21): The existing test-rpc-multi-server target at lines 7-15 inside the if (LLAMA_BUILD_TESTS AND UNIX AND NOT GGML_BACKEND_DL) block establishes the exact pattern: add_executable → target_link_libraries → target_include_directories → add_test → set_property. The hunk (F5.H1): Adds test-rpc-malformed-graph following that same pattern identically — same guard block, same link libraries (ggml ggml-rpc), same include dir (${PROJECT_SOURCE_DIR}/ggml/src), same add_test structure with bash script, same LABELS main. Cross-checks performed: - The source path ${PROJECT_SOURCE_DIR}/tests/test-rpc-malformed-graph.cpp matches the actual file F3. - The shell script path ${PROJECT_SOURCE_DIR}/tests/test-rpc-malformed-graph.sh matches the actual file F4. - The add_test command passes $<TARGET_FILE:ggml-rpc-server> and $<TARGET_FILE:test-rpc-malformed-graph>, which the shell script receives as $1 (server) and $2 (client) — correct. - The shell script calls "$client" "$endpoint", giving the C++ binary one argument (endpoint), matching GGML_ASSERT(argc == 2). - The ggml-rpc.h header is at ggml/include/, brought in transitively by linking ggml-rpc; the explicit ggml/src include dir is harmless (unused by this test but matches convention). - The UNIX guard covers the POSIX dependencies (bash, /dev/tcp, <unistd.h>). - The NOT GGML_BACKEND_DL guard matches the existing test — needed because ggml-rpc may not be a linkable target under dynamic loading. This file is clean. No findings to file. --- Closing lines — F5 tools/rpc/CMakeLists.txt What I checked: - bug: The CMake target definition, test registration, and argument passing are all correct and consistent with the existing test-rpc-multi-server target. No misconfiguration. - convention: The new target is a line-for-line structural copy of the existing pattern (same commands, same order, same guard block). Matches perfectly. - breaks-consumer: N/A — CMakeLists.txt is a leaf build file; no downstream consumers. - duplicate: The wait_for_port bash helper is duplicated between the two test shell scripts, but this is an existing pattern in the repo's test scripts and not introduced by this change's CMake lines. - dead-code / stale-text: No comments or stale references in the CMake lines. - performance / security / compatibility: N/A for a build-system test registration. - tests: The test target is correctly registered with LABELS main and the add_test command properly wires the server and client executables to the shell script. Disposition: Clean — no findings. The CMakeLists.txt change correctly adds the test-rpc-malformed-graph test target following the exact convention of the existing test-rpc-multi-server target, with correct paths, link libraries, and argument passing. What I did not get to: Nothing remaining for this file.
F5.H1
if (LLAMA_BUILD_TESTS AND UNIX AND NOT GGML_BACKEND_DL)
1313 $<TARGET_FILE:ggml-rpc-server>
1414 $<TARGET_FILE:test-rpc-multi-server>)
1515 set_property(TEST test-rpc-multi-server PROPERTY LABELS main)
16+
17+ add_executable(test-rpc-malformed-graph ${PROJECT_SOURCE_DIR}/tests/test-rpc-malformed-graph.cpp)
18+ target_link_libraries(test-rpc-malformed-graph PRIVATE ggml ggml-rpc)
19+ target_include_directories(test-rpc-malformed-graph PRIVATE ${PROJECT_SOURCE_DIR}/ggml/src)
20+ add_test(
21+ NAME test-rpc-malformed-graph
22+ COMMAND bash ${PROJECT_SOURCE_DIR}/tests/test-rpc-malformed-graph.sh
23+ $<TARGET_FILE:ggml-rpc-server>
24+ $<TARGET_FILE:test-rpc-malformed-graph>)
25+ set_property(TEST test-rpc-malformed-graph PROPERTY LABELS main)
1626endif()
1727
1828if(LLAMA_TOOLS_INSTALL)