Merge pull request #52493 from mtaufen/fix-file-leak

Automatic merge from submit-queue (batch tested with PRs 52721, 53057, 52493, 52998, 52896). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

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.

See: https://github.com/kubernetes/kubernetes/pull/52119#discussion_r137916659
Fixes: #53060

```release-note
NONE
```

@yujuhong @ash2k

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
+// WriteTmpFile creates a temporary file at `path`, writes `data` into it, and fsyncs the file
-// dir as `path` and renaming the tmp file over `path`. The file does not have to exist to use ReplaceFile.
+func WriteTmpFile(fs utilfs.Filesystem, path string, data []byte) (tmpPath string, retErr error) {
-func ReplaceFile(fs utilfs.Filesystem, path string, data []byte) 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 tmpPath, tmpFile.Sync()
-		return err
+}
-	}
-	if err := tmpFile.Close(); err != nil {
-		return err
-	}
 
+// 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

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)

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

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)