From aeafe668d8b6d25caac671d59e2b0f0473ce35d0 Mon Sep 17 00:00:00 2001
From: Brendan Ashworth <brendanashworth@users.noreply.github.com>
Date: Fri, 4 Aug 2017 16:51:20 -0700
Subject: [PATCH] posix: do not upstream errors in deleteFile (#4771)

This commit changes posix's deleteFile() to not upstream errors from
removing parent directories. This fixes a race condition.

The race condition occurs when multiple deleteFile()s are called on the
same parent directory, but different child files. Because deleteFile()
recursively removes parent directories if they are empty, but
deleteFile() errors if the selected deletePath does not exist, there was
an opportunity for a race condition. The two processes would remove the
child directories successfully, then depend on the parent directory
still existing. In some cases this is an invalid assumption, because
other processes can remove the parent directory beforehand. This commit
changes deleteFile() to not upstream an error if one occurs, because the
only required error should be from the immediate deletePath, not from a
parent path.

In the specific bug report, multiple CompleteMultipartUpload requests
would launch multiple deleteFile() requests. Because they chain up on
parent directories, ultimately at the end, there would be multiple
remove files for the ultimate parent directory,
.minio.sys/multipart/{bucket}. Because only one will succeed and one
will fail, an error would be upstreamed saying that the file does not
exist, and the CompleteMultipartUpload code interpreted this as
NoSuchKey, or that the object/part id doesn't exist. This was faulty
behavior and is now fixed.

The added test fails before this change and passes after this change.

Fixes: https://github.com/minio/minio/issues/4727
---
 cmd/posix.go      |  5 ++++-
 cmd/posix_test.go | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/cmd/posix.go b/cmd/posix.go
index b777a1b3e..7db081fae 100644
--- a/cmd/posix.go
+++ b/cmd/posix.go
@@ -912,7 +912,10 @@ func deleteFile(basePath, deletePath string) error {
 	}
 
 	// Recursively go down the next path and delete again.
-	return deleteFile(basePath, slashpath.Dir(deletePath))
+	// Errors for parent directories shouldn't trickle down.
+	deleteFile(basePath, slashpath.Dir(deletePath))
+
+	return nil
 }
 
 // DeleteFile - delete a file at path.
diff --git a/cmd/posix_test.go b/cmd/posix_test.go
index d5d48c70d..27817afb2 100644
--- a/cmd/posix_test.go
+++ b/cmd/posix_test.go
@@ -742,6 +742,17 @@ func TestPosixDeleteFile(t *testing.T) {
 		t.Fatalf("Unable to create file, %s", err)
 	}
 
+	if err = posixStorage.MakeVol("no-permissions"); err != nil {
+		t.Fatalf("Unable to create volume, %s", err.Error())
+	}
+	if err = posixStorage.AppendFile("no-permissions", "dir/file", []byte("Hello, world")); err != nil {
+		t.Fatalf("Unable to create file, %s", err.Error())
+	}
+	// Parent directory must have write permissions, this is read + execute.
+	if err = os.Chmod(pathJoin(path, "no-permissions"), 0555); err != nil {
+		t.Fatalf("Unable to chmod directory, %s", err.Error())
+	}
+
 	testCases := []struct {
 		srcVol      string
 		srcPath     string
@@ -796,6 +807,15 @@ func TestPosixDeleteFile(t *testing.T) {
 			ioErrCnt:    0,
 			expectedErr: errFileNameTooLong,
 		},
+		// TestPosix case - 7.
+		// TestPosix case with undeletable parent directory.
+		// File can delete, dir cannot delete because no-permissions doesn't have write perms.
+		{
+			srcVol:      "no-permissions",
+			srcPath:     "dir/file",
+			ioErrCnt:    0,
+			expectedErr: nil,
+		},
 	}
 
 	for i, testCase := range testCases {