From 356118694b4349258a93b5e4bdf3cad57f0d218b Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Sat, 16 May 2020 11:42:01 -0700
Subject: [PATCH 1/2] MkdirAllNewAs: error out if dir exists as file

os.MkdirAll() function returns "not a directory" error in case a
directory to be created already exists but is not a directory
(e.g. a file). MkdirAllNewAs function do not replicate the behavior.

This is a bug since it is expected to ensure the required directory
exists and is indeed a directory, and return an error otherwise.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 idtools.go | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/idtools.go b/idtools.go
index 161aec8..685815d 100644
--- a/idtools.go
+++ b/idtools.go
@@ -3,6 +3,7 @@ package fileutils
 import (
 	"os"
 	"path/filepath"
+	"syscall"
 )
 
 // MkdirAllNewAs creates a directory (include any along the path) and then modifies
@@ -14,9 +15,13 @@ func MkdirAllNewAs(path string, mode os.FileMode, ownerUID, ownerGID int) error
 	// so that we can chown all of them properly at the end.  If chownExisting is false, we won't
 	// chown the full directory path if it exists
 	var paths []string
-	if _, err := os.Stat(path); err != nil && os.IsNotExist(err) {
+	st, err := os.Stat(path)
+	if err != nil && os.IsNotExist(err) {
 		paths = []string{path}
 	} else if err == nil {
+		if !st.IsDir() {
+			return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
+		}
 		// nothing to do; directory path fully exists already
 		return nil
 	}

From 5f27393c553a4faf32f4143074c5c72092af55d5 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Sat, 16 May 2020 11:44:53 -0700
Subject: [PATCH 2/2] idtools: fix MkdirAll usage

This subtle bug keeps lurking in because error checking for `Mkdir()`
and `MkdirAll()` is slightly different wrt `EEXIST`/`IsExist`:

 - for `Mkdir()`, `IsExist` error should (usually) be ignored
   (unless you want to make sure directory was not there before)
   as it means "the destination directory was already there";

 - for `MkdirAll()`, `IsExist` error should NEVER be ignored.

This commit removes ignoring the IsExist error, as it should not
be ignored.

For more details, a quote from my runc commit 6f82d4b (July 2015):

    TL;DR: check for IsExist(err) after a failed MkdirAll() is both
    redundant and wrong -- so two reasons to remove it.

    Quoting MkdirAll documentation:

    > MkdirAll creates a directory named path, along with any necessary
    > parents, and returns nil, or else returns an error. If path
    > is already a directory, MkdirAll does nothing and returns nil.

    This means two things:

    1. If a directory to be created already exists, no error is
    returned.

    2. If the error returned is IsExist (EEXIST), it means there exists
    a non-directory with the same name as MkdirAll need to use for
    directory. Example: we want to MkdirAll("a/b"), but file "a"
    (or "a/b") already exists, so MkdirAll fails.

    The above is a theory, based on quoted documentation and my UNIX
    knowledge.

    3. In practice, though, current MkdirAll implementation [1] returns
    ENOTDIR in most of cases described in #2, with the exception when
    there is a race between MkdirAll and someone else creating the
    last component of MkdirAll argument as a file. In this very case
    MkdirAll() will indeed return EEXIST.

    Because of #1, IsExist check after MkdirAll is not needed.

    Because of #2 and #3, ignoring IsExist error is just plain wrong,
    as directory we require is not created. It's cleaner to report
    the error now.

    Note this error is all over the tree, I guess due to copy-paste,
    or trying to follow the same usage pattern as for Mkdir(),
    or some not quite correct examples on the Internet.

    [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 idtools.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/idtools.go b/idtools.go
index 685815d..bad6539 100644
--- a/idtools.go
+++ b/idtools.go
@@ -39,7 +39,7 @@ func MkdirAllNewAs(path string, mode os.FileMode, ownerUID, ownerGID int) error
 		}
 	}
 
-	if err := os.MkdirAll(path, mode); err != nil && !os.IsExist(err) {
+	if err := os.MkdirAll(path, mode); err != nil {
 		return err
 	}