Skip to content

cmd: fix limitedRead returning nil error when Stat fails - #2905

Open
Nilesh Patil (nileshpatil6) wants to merge 1 commit into
microsoft:mainfrom
nileshpatil6:fix/limited-read-nil-error
Open

cmd: fix limitedRead returning nil error when Stat fails#2905
Nilesh Patil (nileshpatil6) wants to merge 1 commit into
microsoft:mainfrom
nileshpatil6:fix/limited-read-nil-error

Conversation

@nileshpatil6

Copy link
Copy Markdown

Summary

limitedRead in cmd/containerd-shim-runhcs-v1/delete.go can return a nil error on failure. The Stat result is checked inside an if statement, so its err is scoped to that statement, and the err in the final return refers to the outer variable from os.Open, which is always nil once execution gets there:

f, err := os.Open(filePath)            // err == nil past this check
...
if fi, err := f.Stat(); err == nil {   // shadows err
    ...
}
return []byte{}, errors.Wrapf(err, "limited read failed during file stat: %s", filePath)

errors.Wrapf returns nil for a nil error, so a Stat failure produces ([]byte{}, nil). The caller in the delete handler then takes neither branch: err == nil && len(logBytes) > 0 is false, and err != nil is false, so the shim panic log is silently skipped without even the "failed to open shim panic log" warning.

Change

  • Restructure the function the way the containerd-shim-lcow-v2 copy (cmd/containerd-shim-lcow-v2/manager.go) already does, so the Stat error is actually returned.
  • Switch both copies from f.Read(buf) to io.ReadFull with the read count sliced off. f.Read may return fewer bytes than requested without an error, and the old code returned the full buffer regardless, so a short read handed back NUL padding that was never in the file. Same reasoning as ext4/dmverity: use io.ReadFull when reading the super block and root hash #2888.
  • Add the same limitedRead unit tests the lcow shim already has, since the runhcs-v1 copy had none.

Verification

go build ./cmd/containerd-shim-runhcs-v1/          ok
go vet   ./cmd/containerd-shim-runhcs-v1/          ok
go test  ./cmd/containerd-shim-runhcs-v1/ -run TestLimitedRead   PASS
go build -tags lcow ./cmd/containerd-shim-lcow-v2/ ok
go test  -tags lcow ./cmd/containerd-shim-lcow-v2/ -run TestLimitedRead   ok

on Windows 11.

In containerd-shim-runhcs-v1, limitedRead checked the Stat result inside
an if statement, so the err in the final return referred to the outer err
from os.Open, which is always nil at that point. errors.Wrapf returns nil
for a nil error, so a Stat failure returned ([]byte{}, nil) and the delete
handler silently skipped the shim panic log without logging a warning.

Restructure the function the way the containerd-shim-lcow-v2 copy already
does, and switch both copies from f.Read to io.ReadFull with the read
count sliced off, so a short read cannot hand back NUL padding that was
never in the file. Add the same limitedRead unit tests the lcow shim has.

Signed-off-by: nileshpatil6 <technil6436@gmail.com>
@nileshpatil6
Nilesh Patil (nileshpatil6) requested a review from a team as a code owner August 31, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant