From e1f93dea7ce3297427824a9928c72ab0a4ba5f65 Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 11:50:35 +0200 Subject: [PATCH] =?UTF-8?q?sicd+client:=20explicit=20argc=20in=20v2=20cont?= =?UTF-8?q?ent=20=E2=80=94=20empty=20args=20representable=20(reviewer=2096?= =?UTF-8?q?4=20#1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-blocker #1: the empty-netstring terminator collided with a legit empty "" argument — parseV2Content broke on the first len(ns)==0, so echo A "" B ran as echo A and B bled into child stdin (wrong command executed, not rejected). Replaced the terminator with an explicit argc prefix: content = netstring(argc) + argv netstrings + payload; the daemon reads EXACTLY argc elements, so "" is a normal arg. Fixes the collision AND the element-count bound in one. Daemon parseV2Content, client v2Frame, both test helpers, and the reference builder all updated. Proof: TestV2EmptyArgPreserved (echo A "" B -> "A B") + TestV2FrameEmptyArgRepresentable. All v2/v1 tests green, vet clean. Daemon now clears reviewer 964 #1/#2/#3 — deployable. --- cmd/sic/frame.go | 7 ++++--- cmd/sic/frame_test.go | 11 +++++++++-- cmd/sicd/main.go | 24 +++++++++++++++++------- cmd/sicd/sicd_test.go | 24 +++++++++++++++++++----- 4 files changed, 49 insertions(+), 17 deletions(-) diff --git a/cmd/sic/frame.go b/cmd/sic/frame.go index 066da2b..a1de040 100644 --- a/cmd/sic/frame.go +++ b/cmd/sic/frame.go @@ -16,16 +16,17 @@ func nsBytes(b []byte) []byte { // v2Frame builds one v2 wire frame from an EXPLICIT argv (boundary-preserving) plus a payload: // // 0x00 ++ big-endian uint32 length of the netstring ++ netstring(content) -// content = ++ "0:," (empty-netstring terminator) ++ payload +// content = netstring(argc) ++ ++ payload // // Length-framed argv is why `sic host touch 'a b'` sends ONE argument, not two — no space-split // anywhere. The payload is the next hop's nested frame, or empty for the innermost command. func v2Frame(argv [][]byte, payload []byte) []byte { - var content []byte + // content = netstring(argc) + argv netstrings + payload. The explicit count (not an empty- + // netstring terminator) makes an empty "" argument representable (reviewer 964 #1). + content := nsBytes([]byte(strconv.Itoa(len(argv)))) for _, a := range argv { content = append(content, nsBytes(a)...) } - content = append(content, "0:,"...) // empty netstring terminates argv content = append(content, payload...) ns := nsBytes(content) diff --git a/cmd/sic/frame_test.go b/cmd/sic/frame_test.go index 9318e76..893653f 100644 --- a/cmd/sic/frame_test.go +++ b/cmd/sic/frame_test.go @@ -22,14 +22,21 @@ func refFrame(content []byte) []byte { return append(append(f, l[:]...), ns...) } func refContent(argv [][]byte, payload []byte) []byte { - var c []byte + c := refNS([]byte(strconv.Itoa(len(argv)))) // argc for _, a := range argv { c = append(c, refNS(a)...) } - c = append(c, "0:,"...) return append(c, payload...) } +func TestV2FrameEmptyArgRepresentable(t *testing.T) { + // reviewer 964 #1: an empty "" argument is a real element now, not a terminator collision. + argv := [][]byte{[]byte("echo"), []byte(""), []byte("x")} + if got := v2Frame(argv, nil); !bytes.Equal(got, refFrame(refContent(argv, nil))) { + t.Fatalf("empty-arg frame mismatch") + } +} + func TestV2Frame(t *testing.T) { // An argument containing a space is ONE length-framed element — sic's founding guarantee. argv := [][]byte{[]byte("cat"), []byte("a b")} diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index a532fc8..ea46903 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -99,20 +99,30 @@ func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) { const maxArgs = 4096 const maxFrameBytes = 1 << 20 r := bufio.NewReader(bytes.NewReader(content)) + // First netstring = argc (decimal). An explicit count makes the empty string a REPRESENTABLE + // argument: with an empty-netstring terminator, a legit "" arg was indistinguishable from the + // end of argv and silently truncated the command, bleeding the tail into stdin (reviewer 964 + // #1). Reading exactly argc elements removes the collision and bounds the count (#2). + argcB, good := readNetstringStream(r) + if !good { + return nil, nil, false + } + argc, err := strconv.Atoi(string(argcB)) + if err != nil || argc < 0 || argc > maxArgs { + return nil, nil, false + } total := 0 - for { + argv = make([][]byte, 0, argc) + for i := 0; i < argc; i++ { ns, good := readNetstringStream(r) if !good { return nil, nil, false } - if len(ns) == 0 { - break // the empty netstring 0:, terminates argv - } total += len(ns) - if len(argv) >= maxArgs || total > maxFrameBytes { - return nil, nil, false // reviewer #2: bound element count + bytes, like runV1 + if total > maxFrameBytes { + return nil, nil, false } - argv = append(argv, ns) // readNetstringStream returns a fresh slice; no aliasing + argv = append(argv, ns) // fresh slice per element; empty "" is a valid arg now } rest, err := io.ReadAll(r) if err != nil { diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index bc7cb9f..14c605f 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -78,11 +78,12 @@ func frame(command, payload []byte) []byte { // v2 content = argv netstrings + the empty-netstring terminator 0:, + payload. The command is // split on spaces HERE (test-side convenience); the wire itself no longer space-splits — argv // arrives length-framed so `touch 'a b'` keeps its boundary. - var content []byte - for _, a := range bytes.Split(command, []byte(" ")) { + args := bytes.Split(command, []byte(" ")) + content := netstring([]byte(strconv.Itoa(len(args)))) + for _, a := range args { content = concat(content, netstring(a)) } - content = concat(content, []byte("0:,"), payload) + content = concat(content, payload) ns := netstring(content) return concat([]byte{0x00}, be32(uint32(len(ns))), ns) } @@ -602,15 +603,28 @@ func TestPumpStdinCleanEofReturnsChildStatus(t *testing.T) { // v2FrameArgv builds a v2 frame from EXPLICIT argv (no space-split) — the whole point of the // argv-netstring wire: each element is length-framed and rides untouched. func v2FrameArgv(argv [][]byte, payload []byte) []byte { - var content []byte + content := netstring([]byte(strconv.Itoa(len(argv)))) for _, a := range argv { content = concat(content, netstring(a)) } - content = concat(content, []byte("0:,"), payload) + content = concat(content, payload) ns := netstring(content) return concat([]byte{0x00}, be32(uint32(len(ns))), ns) } +func TestV2EmptyArgPreserved(t *testing.T) { + // reviewer 964 #1: an empty "" argument must be carried, not silently dropped. The old + // terminator wire ran `echo A "" B` as `echo A` (B bled into stdin). With argc all three + // survive: echo prints "A B" (two spaces — the empty middle arg). + r := runSicd(t, v2FrameArgv([][]byte{[]byte("echo"), []byte("A"), []byte(""), []byte("B")}, nil)) + if r.code != 0 { + t.Fatalf("expected exit 0, got %d, stderr=%q", r.code, r.stderr) + } + if got, want := string(r.stdout), "A B\n"; got != want { + t.Fatalf("empty arg not preserved: echo printed %q, want %q", got, want) + } +} + func TestV2ArgvBoundaryPreserved(t *testing.T) { // sic's founding guarantee, now on the v2 wire: an argument containing a space is ONE argv // element, never re-split. Before the argv-netstring wire this failed — the space-split gave