merge #35 into cyphar/filepath-securejoin:main

Aleksa Sarai (3):
  mkdir: add racing MkdirAll test
  mkdir: do not error out with -EEXIST for racing MkdirAlls
  gha: bump go test timeout to 30m

LGTMs: cyphar

.github/workflows/ci.yml

@@ -71,9 +71,9 @@ jobs:
           GOCOVERDIR="$(mktemp --tmpdir -d gocoverdir.XXXXXXXX)"
           echo "GOCOVERDIR=$GOCOVERDIR" >>"$GITHUB_ENV"
       - name: go test
-        run: go test -v -cover -test.gocoverdir="$GOCOVERDIR" ./...
+        run: go test -v -cover -timeout=30m -test.gocoverdir="$GOCOVERDIR" ./...
       - name: sudo go test
-        run: sudo go test -v -cover -test.gocoverdir="$GOCOVERDIR" ./...
+        run: sudo go test -v -cover -timeout=30m -test.gocoverdir="$GOCOVERDIR" ./...
       - name: upload coverage
         uses: actions/upload-artifact@v4
         with:

CHANGELOG.md

@@ -5,6 +5,11 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
+### Fixed ###
+- `MkdirAll` will now no longer return an `EEXIST` error if two racing
+  processes are creating the same directory. We will still verify that the path
+  is a directory, but this will avoid spurious errors when multiple threads or
+  programs are trying to `MkdirAll` the same path. opencontainers/runc#4543
 
 ## [0.3.4] - 2024-10-09 ##
 

mkdir_linux.go

@@ -119,7 +119,12 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		// NOTE: mkdir(2) will not follow trailing symlinks, so we can safely
 		// create the final component without worrying about symlink-exchange
 		// attacks.
-		if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil {
+		//
+		// If we get -EEXIST, it's possible that another program created the
+		// directory at the same time as us. In that case, just continue on as
+		// if we created it (if the created inode is not a directory, the
+		// following open call will fail).
+		if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil && !errors.Is(err, unix.EEXIST) {
 			err = &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err}
 			// Make the error a bit nicer if the directory is dead.
 			if err2 := isDeadInode(currentDir); err2 != nil {

mkdir_linux_test.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -149,14 +150,14 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) {
 			"nondir-symlink-dotdot":   {unsafePath: "b-file/../d", expectedErr: unix.ENOTDIR},
 			"nondir-symlink-subdir":   {unsafePath: "b-file/subdir", expectedErr: unix.ENOTDIR},
 			// Dangling symlinks are not followed.
-			"dangling1-trailing": {unsafePath: "a-fake1", expectedErr: unix.EEXIST},
-			"dangling1-basic":    {unsafePath: "a-fake1/foo", expectedErr: unix.EEXIST},
+			"dangling1-trailing": {unsafePath: "a-fake1", expectedErr: unix.ENOTDIR},
+			"dangling1-basic":    {unsafePath: "a-fake1/foo", expectedErr: unix.ENOTDIR},
 			"dangling1-dotdot":   {unsafePath: "a-fake1/../bar/baz", expectedErr: unix.ENOENT},
-			"dangling2-trailing": {unsafePath: "a-fake2", expectedErr: unix.EEXIST},
-			"dangling2-basic":    {unsafePath: "a-fake2/foo", expectedErr: unix.EEXIST},
+			"dangling2-trailing": {unsafePath: "a-fake2", expectedErr: unix.ENOTDIR},
+			"dangling2-basic":    {unsafePath: "a-fake2/foo", expectedErr: unix.ENOTDIR},
 			"dangling2-dotdot":   {unsafePath: "a-fake2/../bar/baz", expectedErr: unix.ENOENT},
-			"dangling3-trailing": {unsafePath: "a-fake3", expectedErr: unix.EEXIST},
-			"dangling3-basic":    {unsafePath: "a-fake3/foo", expectedErr: unix.EEXIST},
+			"dangling3-trailing": {unsafePath: "a-fake3", expectedErr: unix.ENOTDIR},
+			"dangling3-basic":    {unsafePath: "a-fake3/foo", expectedErr: unix.ENOTDIR},
 			"dangling3-dotdot":   {unsafePath: "a-fake3/../bar/baz", expectedErr: unix.ENOENT},
 			// Non-lexical symlinks should work.
 			"nonlexical-basic":           {unsafePath: "target/foo"},
@@ -171,11 +172,11 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) {
 			"nonlexical-level3-abs":      {unsafePath: "link3/target_abs/foo"},
 			"nonlexical-level3-rel":      {unsafePath: "link3/target_rel/foo"},
 			// But really tricky dangling symlinks should fail.
-			"dangling-tricky1-trailing": {unsafePath: "link3/deep_dangling1", expectedErr: unix.EEXIST},
-			"dangling-tricky1-basic":    {unsafePath: "link3/deep_dangling1/foo", expectedErr: unix.EEXIST},
+			"dangling-tricky1-trailing": {unsafePath: "link3/deep_dangling1", expectedErr: unix.ENOTDIR},
+			"dangling-tricky1-basic":    {unsafePath: "link3/deep_dangling1/foo", expectedErr: unix.ENOTDIR},
 			"dangling-tricky1-dotdot":   {unsafePath: "link3/deep_dangling1/../bar", expectedErr: unix.ENOENT},
-			"dangling-tricky2-trailing": {unsafePath: "link3/deep_dangling2", expectedErr: unix.EEXIST},
-			"dangling-tricky2-basic":    {unsafePath: "link3/deep_dangling2/foo", expectedErr: unix.EEXIST},
+			"dangling-tricky2-trailing": {unsafePath: "link3/deep_dangling2", expectedErr: unix.ENOTDIR},
+			"dangling-tricky2-basic":    {unsafePath: "link3/deep_dangling2/foo", expectedErr: unix.ENOTDIR},
 			"dangling-tricky2-dotdot":   {unsafePath: "link3/deep_dangling2/../bar", expectedErr: unix.ENOENT},
 			// And trying to mkdir inside a loop should fail.
 			"loop-trailing": {unsafePath: "loop/link", expectedErr: unix.ELOOP},
@@ -348,7 +349,7 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) {
 			{"trailing", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e", nil},
 			{"partial", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
 			{"trailing", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e", nil},
-			{"partial", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{unix.EEXIST}},
+			{"partial", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{unix.ENOTDIR}},
 			{"trailing", "target/a/b/c/d/e", "swapfile", "target/a/b/c/d/e", []error{unix.ENOTDIR}},
 			{"partial", "target/a/b/c/d/e", "swapfile", "target/a/b/c/d/e/f/g/h/i/j/k", []error{unix.ENOTDIR}},
 		}
@@ -497,3 +498,45 @@ func TestMkdirAllHandle_RacingDelete(t *testing.T) {
 		}
 	})
 }
+
+// Regression test for <https://github.com/opencontainers/runc/issues/4543>.
+func TestMkdirAllHandle_RacingCreate(t *testing.T) {
+	withWithoutOpenat2(t, false, func(t *testing.T) {
+		threadRanges := []int{2, 4, 8, 16, 32, 64, 128, 512, 1024}
+		for _, numThreads := range threadRanges {
+			numThreads := numThreads
+			t.Run(fmt.Sprintf("threads=%d", numThreads), func(t *testing.T) {
+				// Do several runs to try to catch bugs.
+				const testRuns = 500
+				m := newRacingMkdirMeta()
+				for i := 0; i < testRuns; i++ {
+					root := t.TempDir()
+					unsafePath := "a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/x/y/z"
+
+					// Spawn many threads that will race against each other to
+					// create the same directory.
+					startCh := make(chan struct{})
+					var finishedWg sync.WaitGroup
+					for i := 0; i < numThreads; i++ {
+						finishedWg.Add(1)
+						go func() {
+							<-startCh
+							m.checkMkdirAllHandle_Racing(t, root, unsafePath, 0o711, nil)
+							finishedWg.Done()
+						}()
+					}
+
+					// Start all of the threads at the same time.
+					close(startCh)
+
+					// Wait for all of the racing threads to finish.
+					finishedWg.Wait()
+
+					// Clean up the root after each run so we don't exhaust all
+					// space in the tmpfs.
+					_ = os.RemoveAll(root)
+				}
+			})
+		}
+	})
+}