From 9840e91da190f8d3f34fbf195aaa42f30b440e9d Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 22 Jun 2026 10:48:47 -0700 Subject: [PATCH] [internal/wclayer]: clear currentIsDir in reset deferred cleanup When reset returns an error from inside the currentIsDir block (e.g. safefile.RemoveRelative or br.Next fails), the deferred cleanup closes currentFile and sets it to nil, but previously left currentIsDir true. A subsequent call to reset (e.g. from Close after a failed Add) would then enter the currentIsDir block, assign r := w.currentFile (nil), and panic at r.Seek. Fix: set w.currentIsDir = false in the deferred cleanup alongside the other currentFile fields so all file-related state is consistently cleared even on the error path. Adds a test covering this exact scenario: error inside the currentIsDir block, followed by a second reset call that must not panic. Assisted-by: Github-Copilot Signed-off-by: Maksim An --- internal/wclayer/legacy.go | 1 + internal/wclayer/legacy_test.go | 55 ++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/internal/wclayer/legacy.go b/internal/wclayer/legacy.go index ea40e4f7ff..f981ed34c0 100644 --- a/internal/wclayer/legacy.go +++ b/internal/wclayer/legacy.go @@ -453,6 +453,7 @@ func (w *legacyLayerWriter) reset() (err error) { w.currentFile = nil w.currentFileName = "" w.currentFileRoot = nil + w.currentIsDir = false } }() diff --git a/internal/wclayer/legacy_test.go b/internal/wclayer/legacy_test.go index fc5de3daed..9bf51b2478 100644 --- a/internal/wclayer/legacy_test.go +++ b/internal/wclayer/legacy_test.go @@ -60,7 +60,60 @@ func Test_legacyLayerWriter_reset_ClosesFileOnFlushError(t *testing.T) { } } -// Test_legacyLayerWriter_reset_ClosesFileOnSuccess verifies the normal path also +// Test_legacyLayerWriter_reset_ClearsCurrentIsDirOnError verifies that +// currentIsDir is cleared by the deferred cleanup when an error occurs inside +// the currentIsDir handling block. Before the fix, currentIsDir remained true +// after the defer closed currentFile, so the next call to reset would enter the +// currentIsDir block and panic with a nil pointer dereference on r.Seek. +func Test_legacyLayerWriter_reset_ClearsCurrentIsDirOnError(t *testing.T) { + dir := t.TempDir() + fpath := filepath.Join(dir, "current") + f, err := os.Create(fpath) + if err != nil { + t.Fatalf("failed to create temp file: %v", err) + } + + // Write non-zero content so Flush succeeds but the backup stream Seek + // returns a file-level error (empty file means Seek succeeds but the + // backup-stream reader immediately sees EOF before returning an error from + // the reparse-data path; writing garbage bytes causes winio to return an + // error from Next()). + if _, err := f.WriteString("not-a-valid-backup-stream"); err != nil { + f.Close() + t.Fatalf("failed to write to temp file: %v", err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + f.Close() + t.Fatalf("failed to seek temp file: %v", err) + } + + w := &legacyLayerWriter{ + currentFile: f, + currentIsDir: true, + bufWriter: bufio.NewWriter(io.Discard), + } + + // reset should return an error from inside the currentIsDir block. + if err := w.reset(); err == nil { + t.Fatal("expected reset to return an error from the currentIsDir block, got nil") + } + + // The critical invariant: currentIsDir must be cleared by the deferred + // cleanup so that a subsequent call to reset does not panic. + if w.currentIsDir { + t.Error("expected currentIsDir to be false after reset error, got true") + } + if w.currentFile != nil { + t.Error("expected currentFile to be nil after reset error") + } + + // A second call to reset must not panic (nil-deref on r.Seek was the bug). + w.bufWriter = bufio.NewWriter(io.Discard) + if err := w.reset(); err != nil { + t.Errorf("second reset call returned unexpected error: %v", err) + } +} + // closes and clears the current file handle. func Test_legacyLayerWriter_reset_ClosesFileOnSuccess(t *testing.T) { dir := t.TempDir()