From 04c64dd1474aedeb3ffd282873084f681dcfc47f Mon Sep 17 00:00:00 2001 From: Maksim An Date: Wed, 17 Jun 2026 00:19:41 -0700 Subject: [PATCH] [internal/wclayer]: fix import temp dir leak on layer extract failure When layer extraction fails because the disk is full, legacyLayerWriter.reset returned early from bufWriter.Flush (ENOSPC) before closing currentFile and backupWriter. On Windows the still-open handle prevented os.RemoveAll from deleting the temporary import directory created by os.MkdirTemp("", "hcs"), which by default (if not explicitly set) resolves to C:\Windows\SystemTemp\hcs*. As a result every failed PullImage layer extract leaked an hcs* directory and compounded disk-space exhaustion that never self-healed. Move the handle-close logic into a deferred func in reset so the file and backup handles are always released, even on the flush-error path, allowing the deferred RemoveAll in legacyLayerWriterWrapper.Close to succeed. Also clean up importPath in NewLayerWriter when newLegacyLayerWriter fails, matching NewLayerReader's exportPath handling. Adds wclayer reset unit tests covering the flush-error and success paths. Co-authored-by: Claude Opus 4.8 Signed-off-by: Maksim An --- internal/wclayer/importlayer.go | 4 ++ internal/wclayer/legacy.go | 34 +++++++----- internal/wclayer/legacy_test.go | 93 +++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 internal/wclayer/legacy_test.go diff --git a/internal/wclayer/importlayer.go b/internal/wclayer/importlayer.go index 8dab72bb2a..67e8321e3f 100644 --- a/internal/wclayer/importlayer.go +++ b/internal/wclayer/importlayer.go @@ -155,6 +155,10 @@ func NewLayerWriter(ctx context.Context, path string, parentLayerPaths []string) } w, err := newLegacyLayerWriter(importPath, parentLayerPaths, path) if err != nil { + // Clean up the temporary import directory so it is not leaked when + // the writer fails to initialize (matches NewLayerReader's handling + // of exportPath). + os.RemoveAll(importPath) return nil, err } return &legacyLayerWriterWrapper{ diff --git a/internal/wclayer/legacy.go b/internal/wclayer/legacy.go index 627060cee4..ea40e4f7ff 100644 --- a/internal/wclayer/legacy.go +++ b/internal/wclayer/legacy.go @@ -436,9 +436,27 @@ func (w *legacyLayerWriter) initUtilityVM() error { return nil } -func (w *legacyLayerWriter) reset() error { - err := w.bufWriter.Flush() - if err != nil { +func (w *legacyLayerWriter) reset() (err error) { + // Always close the current backup writer and file handle, even if an error + // occurs below (e.g. bufWriter.Flush fails with ENOSPC when the disk is + // full). Leaving these handles open on Windows prevents the temporary + // import directory (defaults to SystemTemp, i.e. C:\Windows\SystemTemp\hcs*) + // from being removed by the deferred os.RemoveAll in legacyLayerWriterWrapper.Close, + // which leaks the directory and compounds disk-space exhaustion. + defer func() { + if w.backupWriter != nil { + w.backupWriter.Close() + w.backupWriter = nil + } + if w.currentFile != nil { + w.currentFile.Close() + w.currentFile = nil + w.currentFileName = "" + w.currentFileRoot = nil + } + }() + + if err = w.bufWriter.Flush(); err != nil { return err } w.bufWriter.Reset(io.Discard) @@ -475,16 +493,6 @@ func (w *legacyLayerWriter) reset() error { } w.currentIsDir = false } - if w.backupWriter != nil { - w.backupWriter.Close() - w.backupWriter = nil - } - if w.currentFile != nil { - w.currentFile.Close() - w.currentFile = nil - w.currentFileName = "" - w.currentFileRoot = nil - } return nil } diff --git a/internal/wclayer/legacy_test.go b/internal/wclayer/legacy_test.go new file mode 100644 index 0000000000..fc5de3daed --- /dev/null +++ b/internal/wclayer/legacy_test.go @@ -0,0 +1,93 @@ +//go:build windows + +package wclayer + +import ( + "bufio" + "errors" + "io" + "os" + "path/filepath" + "testing" +) + +// errWriter always fails writes, used to simulate a full disk (ENOSPC) when the +// buffered writer is flushed. +type errWriter struct{} + +func (errWriter) Write(p []byte) (int, error) { + return 0, errors.New("simulated: there is not enough space on the disk") +} + +// Test_legacyLayerWriter_reset_ClosesFileOnFlushError verifies that reset closes +// the current file handle even when bufWriter.Flush fails (e.g. the disk is +// full). Before the fix, an error from Flush returned early and left the file +// handle open, which on Windows prevented the temporary import directory +// (C:\Windows\SystemTemp\hcs*) from being removed, leaking it. +func Test_legacyLayerWriter_reset_ClosesFileOnFlushError(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) + } + + w := &legacyLayerWriter{ + currentFile: f, + // bufWriter wraps a writer that always fails so Flush returns an error. + bufWriter: bufio.NewWriter(errWriter{}), + } + // Buffer some data so Flush actually attempts a (failing) write. + if _, err := w.bufWriter.WriteString("data"); err != nil { + t.Fatalf("failed to buffer data: %v", err) + } + + err = w.reset() + if err == nil { + // Close the handle to avoid leaking it if the expectation is not met. + f.Close() + t.Fatal("expected reset to return the flush error, got nil") + } + + if w.currentFile != nil { + t.Error("expected currentFile to be nil after reset, handle was not closed") + } + + // On Windows an open *os.File cannot be removed (no FILE_SHARE_DELETE), so a + // successful remove proves the handle was actually released by reset. + if err := os.Remove(fpath); err != nil { + t.Errorf("expected temp file to be removable after reset (handle closed), got: %v", err) + } +} + +// Test_legacyLayerWriter_reset_ClosesFileOnSuccess verifies the normal path also +// closes and clears the current file handle. +func Test_legacyLayerWriter_reset_ClosesFileOnSuccess(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) + } + + w := &legacyLayerWriter{ + currentFile: f, + bufWriter: bufio.NewWriter(io.Discard), + } + + if err := w.reset(); err != nil { + f.Close() + t.Fatalf("expected reset to succeed, got: %v", err) + } + + if w.currentFile != nil { + t.Error("expected currentFile to be nil after reset") + } + if w.currentFileName != "" || w.currentFileRoot != nil { + t.Error("expected currentFileName/currentFileRoot to be cleared after reset") + } + + if err := os.Remove(fpath); err != nil { + t.Errorf("expected temp file to be removable after reset (handle closed), got: %v", err) + } +}