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) + } +}