From 62fecfb0f447ae07d2d410cb1da7ae65670b31eb Mon Sep 17 00:00:00 2001
From: Michael Taufen <mtaufen@google.com>
Date: Thu, 14 Sep 2017 09:23:30 -0700
Subject: [PATCH] Fix a potential file leak

Previously, if a write or sync error occurred, we would not have called
Close(). This commit refactors ReplaceFile() so that we are sure to call
Close(), and also attempts to delete the temporary file if errors occur.
---
 pkg/kubelet/kubeletconfig/util/files/files.go | 41 +++++++++++++------
 pkg/util/filesystem/defaultfs.go              |  5 +++
 pkg/util/filesystem/fakefs.go                 |  5 +++
 pkg/util/filesystem/filesystem.go             |  1 +
 4 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/pkg/kubelet/kubeletconfig/util/files/files.go b/pkg/kubelet/kubeletconfig/util/files/files.go
index 62d96fa5ad..a732bbcce8 100644
--- a/pkg/kubelet/kubeletconfig/util/files/files.go
+++ b/pkg/kubelet/kubeletconfig/util/files/files.go
@@ -63,34 +63,51 @@ func EnsureFile(fs utilfs.Filesystem, path string) error {
 	return file.Close()
 }
 
-// ReplaceFile replaces the contents of the file at `path` with `data` by writing to a tmp file in the same
-// dir as `path` and renaming the tmp file over `path`. The file does not have to exist to use ReplaceFile.
-func ReplaceFile(fs utilfs.Filesystem, path string, data []byte) error {
+// WriteTmpFile creates a temporary file at `path`, writes `data` into it, and fsyncs the file
+func WriteTmpFile(fs utilfs.Filesystem, path string, data []byte) (tmpPath string, retErr error) {
 	dir := filepath.Dir(path)
 	prefix := filepath.Base(path)
 
 	// create the tmp file
 	tmpFile, err := fs.TempFile(dir, prefix)
 	if err != nil {
-		return err
+		return "", err
 	}
+	defer func() {
+		// close the file, return the close error only if there haven't been any other errors
+		if err := tmpFile.Close(); retErr == nil {
+			retErr = err
+		}
+		// if there was an error writing, syncing, or closing, delete the temporary file and return the error
+		if retErr != nil {
+			if err := fs.Remove(tmpPath); err != nil {
+				retErr = fmt.Errorf("attempted to remove temporary file %q after error %v, but failed due to error: %v", path, retErr, err)
+			}
+			tmpPath = ""
+		}
+	}()
 
 	// Name() will be an absolute path when using utilfs.DefaultFS, because ioutil.TempFile passes
 	// an absolute path to os.Open, and we ensure similar behavior in utilfs.FakeFS for testing.
-	tmpPath := tmpFile.Name()
+	tmpPath = tmpFile.Name()
 
 	// write data
 	if _, err := tmpFile.Write(data); err != nil {
-		return err
+		return tmpPath, err
 	}
 	// sync file, to ensure it's written in case a hard reset happens
-	if err := tmpFile.Sync(); err != nil {
-		return err
-	}
-	if err := tmpFile.Close(); err != nil {
-		return err
-	}
+	return tmpPath, tmpFile.Sync()
+}
 
+// ReplaceFile replaces the contents of the file at `path` with `data` by writing to a tmp file in the same
+// dir as `path` and renaming the tmp file over `path`. The file does not have to exist to use ReplaceFile.
+// Note ReplaceFile calls fsync.
+func ReplaceFile(fs utilfs.Filesystem, path string, data []byte) error {
+	// write data to a temporary file
+	tmpPath, err := WriteTmpFile(fs, path, data)
+	if err != nil {
+		return err
+	}
 	// rename over existing file
 	if err := fs.Rename(tmpPath, path); err != nil {
 		return err
diff --git a/pkg/util/filesystem/defaultfs.go b/pkg/util/filesystem/defaultfs.go
index ea9d73f139..f87fe3d6df 100644
--- a/pkg/util/filesystem/defaultfs.go
+++ b/pkg/util/filesystem/defaultfs.go
@@ -62,6 +62,11 @@ func (DefaultFs) RemoveAll(path string) error {
 	return os.RemoveAll(path)
 }
 
+// Remove via os.RemoveAll
+func (DefaultFs) Remove(name string) error {
+	return os.Remove(name)
+}
+
 // ReadFile via ioutil.ReadFile
 func (DefaultFs) ReadFile(filename string) ([]byte, error) {
 	return ioutil.ReadFile(filename)
diff --git a/pkg/util/filesystem/fakefs.go b/pkg/util/filesystem/fakefs.go
index 28bbce8067..b103007a55 100644
--- a/pkg/util/filesystem/fakefs.go
+++ b/pkg/util/filesystem/fakefs.go
@@ -92,6 +92,11 @@ func (fs *fakeFs) RemoveAll(path string) error {
 	return fs.a.RemoveAll(path)
 }
 
+// Remove via afero.RemoveAll
+func (fs *fakeFs) Remove(name string) error {
+	return fs.a.Remove(name)
+}
+
 // fakeFile implements File; for use with fakeFs
 type fakeFile struct {
 	file afero.File
diff --git a/pkg/util/filesystem/filesystem.go b/pkg/util/filesystem/filesystem.go
index fcfef1d1f9..16e329b30f 100644
--- a/pkg/util/filesystem/filesystem.go
+++ b/pkg/util/filesystem/filesystem.go
@@ -31,6 +31,7 @@ type Filesystem interface {
 	MkdirAll(path string, perm os.FileMode) error
 	Chtimes(name string, atime time.Time, mtime time.Time) error
 	RemoveAll(path string) error
+	Remove(name string) error
 
 	// from "io/ioutil"
 	ReadFile(filename string) ([]byte, error)