From bf82a80869ad8135e082524536678d8689934eba Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 11:37:54 +0200 Subject: [PATCH] sicd: v2 wire preserves argv boundaries (content = argv netstrings + terminator + payload) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v2 command field was a single string the daemon SPACE-SPLIT into argv — which silently broke sic's founding guarantee (`ssh host touch 'a b'` makes TWO files; `sic host touch 'a b'` must make ONE). The composition of the v2 client surfaced this: every argument with a space would be re-split. Fix (markus chose it): v2 content is now a run of djb netstrings (the argv), terminated by the empty netstring 0:,, then the payload — mirroring the boundary-safe v1 exec encoding. Each argument is length-framed, so spaces, NUL, 0xFF all ride untouched; readNetstringStream already bounds each element + its length token, so a hostile content cannot OOM here. runV2 uses the new parseV2Content; the test frame() helper space-splits test-side only (the wire never does). Proof: TestV2ArgvBoundaryPreserved — argv {script, "a b"} makes the script see 1 arg "a b", not 2. All existing v2 + v1 dual-read tests stay green; go vet clean. Co-Authored-By: Claude Opus 4.8 --- cmd/sicd/main.go | 45 ++++++++++++++++++++++++++++++++----------- cmd/sicd/sicd_test.go | 36 +++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index 89b852b..d1816fb 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -91,6 +91,29 @@ func readNetstringStream(r *bufio.Reader) ([]byte, bool) { return content, true } +// parseV2Content splits a v2 netstring's content into argv (a run of netstrings terminated by +// the empty netstring 0:,) and the trailing payload. Length-framed argv preserves argument +// boundaries and lets any byte (space, NUL, 0xFF) appear in an argument; readNetstringStream +// bounds each element and its length token, so a hostile content cannot OOM here. +func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) { + r := bufio.NewReader(bytes.NewReader(content)) + for { + ns, good := readNetstringStream(r) + if !good { + return nil, nil, false + } + if len(ns) == 0 { + break // the empty netstring 0:, terminates argv + } + argv = append(argv, ns) // readNetstringStream returns a fresh slice; no aliasing + } + rest, err := io.ReadAll(r) + if err != nil { + return nil, nil, false + } + return argv, rest, true +} + func waitForChild(cmd *exec.Cmd) int { if err := cmd.Wait(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { @@ -151,23 +174,23 @@ func runV2() { os.Exit(1) } - nulIdx := bytes.IndexByte(content, 0x00) - if nulIdx < 0 { - fmt.Fprintf(os.Stderr, "sicd: content missing NUL separator\n") + // v2 content = argv netstrings + the empty-netstring terminator 0:, + payload. Length-framed + // argv PRESERVES argument boundaries (sic's founding guarantee: `touch 'a b'` is ONE file) + // and lets ANY byte appear in an argument — unlike the old space-split. Everything after the + // terminator is the payload (a nested frame for the next hop, or empty). + argv, payload, ok := parseV2Content(content) + if !ok { + fmt.Fprintf(os.Stderr, "sicd: invalid v2 content (argv framing)\n") + os.Exit(1) + } + if len(argv) == 0 { + fmt.Fprintf(os.Stderr, "sicd: empty command\n") os.Exit(1) } - command := content[:nulIdx] - payload := content[nulIdx+1:] - - argv := bytes.Split(command, []byte(" ")) argvStr := make([]string, len(argv)) for i, a := range argv { argvStr[i] = string(a) } - if len(argvStr) == 0 { - fmt.Fprintf(os.Stderr, "sicd: empty command\n") - os.Exit(1) - } cmd := exec.Command(argvStr[0], argvStr[1:]...) stdinPipe, err := cmd.StdinPipe() diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index 045500a..5350ea7 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -75,7 +75,15 @@ func be32(n uint32) []byte { // frame builds one wire frame: magic, 4-byte BE length, netstring(command NUL payload). func frame(command, payload []byte) []byte { - ns := netstring(concat(command, []byte{0x00}, payload)) + // 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(" ")) { + content = concat(content, netstring(a)) + } + content = concat(content, []byte("0:,"), payload) + ns := netstring(content) return concat([]byte{0x00}, be32(uint32(len(ns))), ns) } @@ -590,3 +598,29 @@ func TestPumpStdinCleanEofReturnsChildStatus(t *testing.T) { t.Fatalf("a clean EOF is a legitimate end; must return the child's status 7, got %d", code) } } + +// 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 + for _, a := range argv { + content = concat(content, netstring(a)) + } + content = concat(content, []byte("0:,"), payload) + ns := netstring(content) + return concat([]byte{0x00}, be32(uint32(len(ns))), ns) +} + +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 + // the script two args and printed "2|a". This is the regression that pins the fix. + script := shScript(t, `printf '%s|%s\n' "$#" "$1"`) + r := runSicd(t, v2FrameArgv([][]byte{[]byte(script), []byte("a 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), "1|a b\n"; got != want { + t.Fatalf("v2 argv boundary LOST: script saw %q, want %q", got, want) + } +}