kvcache-ai/AgentENV

Fork 0

mirror of https://github.com/kvcache-ai/AgentENV.git synced 2026-11-05 01:17:36 +00:00

[GH-ISSUE #346] bug(plain-snapshotter): corrupted config.json silently wipes all registrations on RegisterRootfs (nil-map panic on null); snap-* temp dirs leak; unknown parent silently yields an empty rootfs #87

New issue

Closed

opened 2026-10-29 14:19:24 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-29 14:19:24 +00:00

Owner

Originally created by @tm-worker on GitHub (Oct 9, 2026).
Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/346

Originally assigned to: @guozy18 on GitHub.

Affected component

containerd-plain-snapshotter (Go proxy snapshotter, containerd-plain-snapshotter/)

AgentENV version

main @ 4968423 (2026-10-12) — first verified on 34cdc80 (2026-10-09); re-confirmed on 4968423 via git diff that the whole containerd-plain-snapshotter/ module is byte-identical between the two revisions, so the findings carry over unchanged. All findings below were exercised on 34cdc80.

Host environment

OS: macOS 15 (darwin/arm64) — also applies to Linux; the module is pure Go with no CGO
Go: go1.26.4
No docker daemon required: the register path was driven with a shim docker on PATH (docker load → exit 0, docker image inspect --format '{{json .RootFS}}' → fixed RootFS JSON), so the tested code path (NewSnapshotter → getConfig → RegisterRootfs) is the real binary
Source modifications: none to module code. Verification ran against an unmodified git-archive copy of the module moved to a scratch directory. The only local additions live outside the repo: the shim docker binary on PATH (a test double emulating the two docker CLI calls) and a ~60-line gRPC probe client (using the module's own containerd/api + grpc dependencies) placed in a scratch copy of the module to drive T3/T4. No file inside containerd-plain-snapshotter/ was changed.

Operation and configuration

Default configuration of the plain snapshotter:

./plain-snapshotter -address /tmp/ps.sock -root /var/lib/containerd-plain-snapshotter
./plain-snapshotter register -root <root> <image> <rootfs-dir>

config.json format is documented in containerd-plain-snapshotter/README.md as { "<chainID>": "<rootfs path>" }, maintained automatically by register.

Steps to reproduce

T1 — corrupted config silently wipes all registrations:

Create a valid config.json: {"sha256:1111...": "/path/rootfsA"}
Corrupt it: printf '[]' > config.json
./plain-snapshotter register -root <root> imgB /path/rootfsB
cat config.json → now contains ONLY the new entry; the old registration is gone

T2 — null config panics:

printf 'null' > config.json
./plain-snapshotter register -root <root> imgC /path/rootfsC
panic: assignment to entry in nil map (snapshotter.go:68)

T3 — snap-* temp dirs leak:

Start the server: ./plain-snapshotter -address /tmp/ps.sock -root <root>
Over gRPC (containerd snapshots API), loop 5×: Prepare(key_i, parent="") then Remove(key_i)
ls <root> → 5 snap-* directories remain; Remove only deletes the in-memory map entry (snapshotter.go:269-275), and there is no GC of <root> anywhere in the module

T4 — unknown parent silently yields an empty rootfs:

With the server running, call Prepare(key="moby/8/sha256:UNKNOWN", parent="sha256:NOTREGISTERED...")
The call succeeds (exit 0) and returns a bind mount pointing at a freshly created empty temp dir (snapshotter.go:203-222 falls through to os.MkdirTemp)

Expected behavior

T1: getConfig should fail loudly on malformed JSON (the register command should exit with an error naming the config file) instead of treating the file as empty and rewriting it.
T2: Same — a JSON error must be reported, not turned into a nil map.
T3: Remove should delete the snap-* directory the snapshotter itself created (it must not touch external rootfs dirs referenced by config.json); a startup sweep for orphaned snap-* dirs would also fix the restart case.
T4: Prepare/View with a non-empty parent that resolves to nothing should return errdefs.ErrNotFound, per the containerd snapshotter contract, instead of an empty-rootfs bind mount.

Actual behavior

T1: json.Unmarshal error is discarded (snapshotter.go:59); getConfig returns an empty map and RegisterRootfs atomically rewrites config.json with only the new entry — every existing chainID→rootfs mapping is silently lost. Subsequent docker run of previously registered images fails in Mounts (snapshotter.go:169-174) with NotFound, with no hint the config was wiped.
T2: JSON null sets the map to nil; config[chainID] = rootfsPath at snapshotter.go:68 panics: assignment to entry in nil map.
T3: Each Prepare that misses both config and parent creates os.MkdirTemp(s.root, "snap-") (snapshotter.go:216-222); Remove deletes only the map entry (snapshotter.go:273). After a snapshotter restart (in-memory map is intentionally cleared), every snap-* dir from the previous run is orphaned permanently; <root> grows unbounded. config-*.tmp files also leak if os.Rename fails (snapshotter.go:87).
T4: Prepare with an unknown parent returns success and an empty rootfs. Combined with T1/T3 this masks root causes: a container started from an unregistered (or wiped) image boots into an empty filesystem and fails later with confusing errors like exec: "/bin/sh": stat ...: no such file or directory.

Logs and diagnostics

T1 (register over [] config):

[DEBUG] loaded config with 0 entries          <- '[]' swallowed, no error
[DEBUG] registered: imgB -> .../rootfsB ...
config.json afterwards:
{
  "sha256:FAKE...AAA": ".../rootfsB"          <- previous entry gone
}

T2 (register over null config):

panic: assignment to entry in nil map

goroutine 1 [running]:
main.(*Snapshotter).RegisterRootfs(...)
	.../snapshotter.go:68 +0x94
main.runRegister()
	.../main.go:112 +0x3e8
exit=2

T3 (5 prepare/remove cycles):

[DEBUG] createSnapshot("leak/test-4") -> rootfs=.../snap-3955177369 (source=tempdir)
[DEBUG] Remove("leak/test-4")
$ ls <root> | grep -c snap-  -> 5

T4 (unknown parent):

[DEBUG] Prepare(key="moby/8/sha256:UNKNOWN", parent="sha256:NOTREGISTERED...")
[DEBUG] createSnapshot(...) -> rootfs=.../snap-3910928569 (source=tempdir)
mount: type=bind source=.../snap-3910928569 options=[rbind rw]   (exit 0)

Reproduction frequency

Always — all four are deterministic (no daemon, no timing involved).

Regression information

Not a regression: the module was introduced recently (5353143 "feat(containerd): add plain snapshotter for running containers from a rootfs directory") and the behaviors have existed since introduction.

Additional context

Lower-severity observations (code-level, not separately reproduced):

Commit (snapshotter.go:241-267) does not check name collisions and silently overwrites an existing committed snapshot (containerd expects ErrAlreadyExists).
Usage (snapshotter.go:156-159) always returns {0,0}, even for unknown keys.
Stat/Walk fabricate Created/Updated: time.Now() for config entries, so timestamps drift.
Mounts (snapshotter.go:167-172) does not verify the registered rootfs dir still exists.

Suggested fix directions:

getConfig: return (map, error); propagate JSON errors with the file path as context.
RegisterRootfs: add a file lock for cross-process safety (the mutex is in-process only).
Remove: delete snap-* dirs the snapshotter created; add a startup sweep for orphaned ones.
createSnapshot: unknown parent → errdefs.ErrNotFound.

Happy to open a PR covering T1-T4 if maintainers agree with the directions.

Pre-submission checklist

I searched existing open and closed issues — and GitHub Discussions (0 related results) — and did not find a duplicate.
I provided a minimal reproduction (T1-T4 above; deterministic, no docker daemon needed).
I removed credentials, tokens, private image references, and other sensitive information.
This is not a security vulnerability.

Originally created by @tm-worker on GitHub (Oct 9, 2026). Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/346 Originally assigned to: @guozy18 on GitHub. ### Affected component `containerd-plain-snapshotter` (Go proxy snapshotter, `containerd-plain-snapshotter/`) ### AgentENV version `main` @ `4968423` (2026-10-12) — first verified on `34cdc80` (2026-10-09); re-confirmed on `4968423` via `git diff` that the whole `containerd-plain-snapshotter/` module is byte-identical between the two revisions, so the findings carry over unchanged. All findings below were exercised on `34cdc80`. ### Host environment - OS: macOS 15 (darwin/arm64) — also applies to Linux; the module is pure Go with no CGO - Go: go1.26.4 - No docker daemon required: the `register` path was driven with a shim `docker` on PATH (`docker load` → exit 0, `docker image inspect --format '{{json .RootFS}}'` → fixed RootFS JSON), so the tested code path (`NewSnapshotter` → `getConfig` → `RegisterRootfs`) is the real binary - **Source modifications: none to module code.** Verification ran against an unmodified git-archive copy of the module moved to a scratch directory. The only local additions live outside the repo: the shim `docker` binary on PATH (a test double emulating the two docker CLI calls) and a ~60-line gRPC probe client (using the module's own `containerd/api` + `grpc` dependencies) placed in a scratch copy of the module to drive T3/T4. No file inside `containerd-plain-snapshotter/` was changed. ### Operation and configuration Default configuration of the plain snapshotter: ```bash ./plain-snapshotter -address /tmp/ps.sock -root /var/lib/containerd-plain-snapshotter ./plain-snapshotter register -root <root> <image> <rootfs-dir> ``` `config.json` format is documented in `containerd-plain-snapshotter/README.md` as `{ "<chainID>": "<rootfs path>" }`, maintained automatically by `register`. ### Steps to reproduce **T1 — corrupted config silently wipes all registrations:** 1. Create a valid `config.json`: `{"sha256:1111...": "/path/rootfsA"}` 2. Corrupt it: `printf '[]' > config.json` 3. `./plain-snapshotter register -root <root> imgB /path/rootfsB` 4. `cat config.json` → now contains ONLY the new entry; the old registration is gone **T2 — `null` config panics:** 1. `printf 'null' > config.json` 2. `./plain-snapshotter register -root <root> imgC /path/rootfsC` 3. `panic: assignment to entry in nil map` (`snapshotter.go:68`) **T3 — `snap-*` temp dirs leak:** 1. Start the server: `./plain-snapshotter -address /tmp/ps.sock -root <root>` 2. Over gRPC (containerd snapshots API), loop 5×: `Prepare(key_i, parent="")` then `Remove(key_i)` 3. `ls <root>` → 5 `snap-*` directories remain; `Remove` only deletes the in-memory map entry (`snapshotter.go:269-275`), and there is no GC of `<root>` anywhere in the module **T4 — unknown parent silently yields an empty rootfs:** 1. With the server running, call `Prepare(key="moby/8/sha256:UNKNOWN", parent="sha256:NOTREGISTERED...")` 2. The call succeeds (exit 0) and returns a bind mount pointing at a freshly created **empty** temp dir (`snapshotter.go:203-222` falls through to `os.MkdirTemp`) ### Expected behavior - **T1**: `getConfig` should fail loudly on malformed JSON (the `register` command should exit with an error naming the config file) instead of treating the file as empty and rewriting it. - **T2**: Same — a JSON error must be reported, not turned into a nil map. - **T3**: `Remove` should delete the `snap-*` directory the snapshotter itself created (it must not touch external rootfs dirs referenced by `config.json`); a startup sweep for orphaned `snap-*` dirs would also fix the restart case. - **T4**: `Prepare`/`View` with a non-empty parent that resolves to nothing should return `errdefs.ErrNotFound`, per the containerd snapshotter contract, instead of an empty-rootfs bind mount. ### Actual behavior - **T1**: `json.Unmarshal` error is discarded (`snapshotter.go:59`); `getConfig` returns an empty map and `RegisterRootfs` atomically rewrites `config.json` with only the new entry — every existing chainID→rootfs mapping is silently lost. Subsequent `docker run` of previously registered images fails in `Mounts` (`snapshotter.go:169-174`) with NotFound, with no hint the config was wiped. - **T2**: JSON `null` sets the map to nil; `config[chainID] = rootfsPath` at `snapshotter.go:68` panics: `assignment to entry in nil map`. - **T3**: Each `Prepare` that misses both config and parent creates `os.MkdirTemp(s.root, "snap-")` (`snapshotter.go:216-222`); `Remove` deletes only the map entry (`snapshotter.go:273`). After a snapshotter restart (in-memory map is intentionally cleared), every `snap-*` dir from the previous run is orphaned permanently; `<root>` grows unbounded. `config-*.tmp` files also leak if `os.Rename` fails (`snapshotter.go:87`). - **T4**: `Prepare` with an unknown parent returns success and an empty rootfs. Combined with T1/T3 this masks root causes: a container started from an unregistered (or wiped) image boots into an empty filesystem and fails later with confusing errors like `exec: "/bin/sh": stat ...: no such file or directory`. ### Logs and diagnostics T1 (register over `[]` config): ```text [DEBUG] loaded config with 0 entries <- '[]' swallowed, no error [DEBUG] registered: imgB -> .../rootfsB ... config.json afterwards: { "sha256:FAKE...AAA": ".../rootfsB" <- previous entry gone } ``` T2 (register over `null` config): ```text panic: assignment to entry in nil map goroutine 1 [running]: main.(*Snapshotter).RegisterRootfs(...) .../snapshotter.go:68 +0x94 main.runRegister() .../main.go:112 +0x3e8 exit=2 ``` T3 (5 prepare/remove cycles): ```text [DEBUG] createSnapshot("leak/test-4") -> rootfs=.../snap-3955177369 (source=tempdir) [DEBUG] Remove("leak/test-4") $ ls <root> | grep -c snap- -> 5 ``` T4 (unknown parent): ```text [DEBUG] Prepare(key="moby/8/sha256:UNKNOWN", parent="sha256:NOTREGISTERED...") [DEBUG] createSnapshot(...) -> rootfs=.../snap-3910928569 (source=tempdir) mount: type=bind source=.../snap-3910928569 options=[rbind rw] (exit 0) ``` ### Reproduction frequency Always — all four are deterministic (no daemon, no timing involved). ### Regression information Not a regression: the module was introduced recently (`5353143` "feat(containerd): add plain snapshotter for running containers from a rootfs directory") and the behaviors have existed since introduction. ### Additional context Lower-severity observations (code-level, not separately reproduced): - `Commit` (`snapshotter.go:241-267`) does not check name collisions and silently overwrites an existing committed snapshot (containerd expects `ErrAlreadyExists`). - `Usage` (`snapshotter.go:156-159`) always returns `{0,0}`, even for unknown keys. - `Stat`/`Walk` fabricate `Created/Updated: time.Now()` for config entries, so timestamps drift. - `Mounts` (`snapshotter.go:167-172`) does not verify the registered rootfs dir still exists. Suggested fix directions: - `getConfig`: return `(map, error)`; propagate JSON errors with the file path as context. - `RegisterRootfs`: add a file lock for cross-process safety (the mutex is in-process only). - `Remove`: delete `snap-*` dirs the snapshotter created; add a startup sweep for orphaned ones. - `createSnapshot`: unknown parent → `errdefs.ErrNotFound`. Happy to open a PR covering T1-T4 if maintainers agree with the directions. ### Pre-submission checklist - [x] I searched existing open and closed issues — and GitHub Discussions (0 related results) — and did not find a duplicate. - [x] I provided a minimal reproduction (T1-T4 above; deterministic, no docker daemon needed). - [x] I removed credentials, tokens, private image references, and other sensitive information. - [x] This is not a security vulnerability.

bsakaguchi closed this issue

2026-10-29 14:19:24 +00:00

bsakaguchi commented

2026-10-29 14:19:24 +00:00

Author

Owner

@Chaoqi31 commented on GitHub (Oct 14, 2026):

Since this issue is assigned, I won't open a PR. Two notes in case they help.

T3 is riskier than it looks. register accepts any directory, so a registered rootfs can be a snap-* dir or sit inside one, and deleting the dir in Remove would take that rootfs with it. A startup sweep adds a second hazard. After a restart the in-memory key map is empty, but containers can still be running on those dirs through their bind mounts. A safe fix probably needs the key-to-dir map persisted, plus a check that no registered rootfs lives under a dir before deleting it. In the supported flow each leaked dir only holds the placeholder layer from docker load, so the leak itself is small.

T1, T2, T4 and the lock. There is a tested patch in one commit on 4968423, limited to containerd-plain-snapshotter/:

getConfig returns an error naming the file for read errors, bad JSON, and non-object JSON. A missing file is still an empty config.
register reads the config before docker load, so a bad config exits 1 with no image loaded and the file untouched. The server refuses to start on it.
RegisterRootfs holds an flock on <root>/config.lock around the read-modify-rename, and removes its temp file if the rename fails.
Prepare/View with a parent that is neither registered nor known in memory return ErrNotFound.

Checked with go test -race -count=20 ./..., a test where 32 snapshotters register at once (without the lock, 1 to 4 of the 32 entries survive), and an end-to-end run of binaries built from main and from the patch, through a shim docker and containerd's snapshots/proxy client. Use any of it if it's useful.

The patch and this note were prepared with Claude Code.

@Chaoqi31 commented on GitHub (Oct 14, 2026): Since this issue is assigned, I won't open a PR. Two notes in case they help. **T3 is riskier than it looks.** `register` accepts any directory, so a registered rootfs can be a `snap-*` dir or sit inside one, and deleting the dir in `Remove` would take that rootfs with it. A startup sweep adds a second hazard. After a restart the in-memory key map is empty, but containers can still be running on those dirs through their bind mounts. A safe fix probably needs the key-to-dir map persisted, plus a check that no registered rootfs lives under a dir before deleting it. In the supported flow each leaked dir only holds the placeholder layer from `docker load`, so the leak itself is small. **T1, T2, T4 and the lock.** There is a tested patch in [one commit on 4968423](https://github.com/Chaoqi31/AgentENV/commit/2a688393cbd803c9b64abe9df410ccaeb54d2a30), limited to `containerd-plain-snapshotter/`: - `getConfig` returns an error naming the file for read errors, bad JSON, and non-object JSON. A missing file is still an empty config. - `register` reads the config before `docker load`, so a bad config exits 1 with no image loaded and the file untouched. The server refuses to start on it. - `RegisterRootfs` holds an `flock` on `<root>/config.lock` around the read-modify-rename, and removes its temp file if the rename fails. - `Prepare`/`View` with a parent that is neither registered nor known in memory return `ErrNotFound`. Checked with `go test -race -count=20 ./...`, a test where 32 snapshotters register at once (without the lock, 1 to 4 of the 32 entries survive), and an end-to-end run of binaries built from main and from the patch, through a shim `docker` and containerd's `snapshots/proxy` client. Use any of it if it's useful. _The patch and this note were prepared with Claude Code._

bsakaguchi commented

2026-10-29 14:19:25 +00:00

Author

Owner

@guozy18 commented on GitHub (Oct 15, 2026):

@tm-worker Thanks for the detailed report and the proposed patch. We've verified and reproduced the behavior you described. However, we'd like to share some context on how plain-snapshotter is scoped and used in AgentENV.

In our current environment:

Isolated lifecycle: Each sandbox runs an independent snapshotter instance with its own state directory, which is completely discarded when the sandbox terminates.
No shared configuration: config.json is generated internally by register, and configurations are never shared across sandboxes.
Sequential registration: Image registration within each sandbox runs strictly sequentially.

Given this lifecycle and initialization flow, our deployment is not exposed to the concurrent-writer lost-update condition. For this reason, we do not plan to adopt file locking at present to keep the implementation minimal.

That said, the issue you identified is completely valid. If we eventually introduce parallel image registration within a single sandbox, we will definitely revisit file locking. We also recognize that standalone setups or deployments with different lifecycle assumptions may have different requirements.

@guozy18 commented on GitHub (Oct 15, 2026): @tm-worker Thanks for the detailed report and the proposed patch. We've verified and reproduced the behavior you described. However, we'd like to share some context on how plain-snapshotter is scoped and used in AgentENV. In our current environment: - **Isolated lifecycle:** Each sandbox runs an independent snapshotter instance with its own state directory, which is completely discarded when the sandbox terminates. - **No shared configuration:** config.json is generated internally by register, and configurations are never shared across sandboxes. - **Sequential registration:** Image registration within each sandbox runs strictly sequentially. Given this lifecycle and initialization flow, our deployment is not exposed to the concurrent-writer lost-update condition. For this reason, we do not plan to adopt file locking at present to keep the implementation minimal. That said, the issue you identified is completely valid. If we eventually introduce parallel image registration within a single sandbox, we will definitely revisit file locking. We also recognize that standalone setups or deployments with different lifecycle assumptions may have different requirements.

bsakaguchi commented

2026-10-29 14:19:25 +00:00

Author

Owner

@tm-worker commented on GitHub (Oct 16, 2026):

Thanks for verifying and for the deployment context — the per-sandbox isolated
instance explains the sequential assumption. Agreed the lock can wait until
parallel registration is actually needed.

@tm-worker commented on GitHub (Oct 16, 2026): Thanks for verifying and for the deployment context — the per-sandbox isolated instance explains the sequential assumption. Agreed the lock can wait until parallel registration is actually needed.

bsakaguchi referenced this issue

2026-10-29 14:20:36 +00:00

[PR #87] [MERGED] feat: support PVM-backed deployment #170