mkdirall: switch to os.FileMode argument

This is mostly a cosmetic change for most users, but libpathrs uses
os.FileMode as well and most Go users are more used to using
os.FileMode. The only thing that users need to watch out for is that
they need to switch from unix.S_ISVTX to os.ModeSticky if they are using
that bit (since os.FileMode and unix.S_* bits have a different layout).

This will also help with building on 32-bit architectures without
switching the argument type to uint32 explicitly.

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

CHANGELOG.md

@@ -22,6 +22,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   necessarily a breaking API change (though we expect no real users to be
   affected by it).
 
+- `MkdirAll` and `MkdirHandle` now take an `os.FileMode`-style mode argument
+  instead of a raw `unix.S_*`-style mode argument, which may cause compile-time
+  type errors depending on how you use `filepath-securejoin`. For most users,
+  there will be no change in behaviour aside from the type change (as the
+  bottom `0o777` bits are the same in both formats, and most users are probably
+  only using those bits).
+
+  However, if you were using `unix.S_ISVTX` to set the sticky bit with
+  `MkdirAll(Handle)` you will need to switch to `os.ModeSticky` otherwise you
+  will get a runtime error with this update. In addition, the error message you
+  will get from passing `unix.S_ISUID` and `unix.S_ISGID` will be different as
+  they are treated as invalid bits now (note that previously passing said bits
+  was also an error).
+
 ## [0.3.6] - 2024-12-17 ##
 
 ### Compatibility ###

mkdir_linux.go

@@ -21,6 +21,33 @@ var (
 	errPossibleAttack = errors.New("possible attack detected")
 )
 
+// modePermExt is like os.ModePerm except that it also includes the set[ug]id
+// and sticky bits.
+const modePermExt = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky
+
+//nolint:cyclop // this function needs to handle a lot of cases
+func toUnixMode(mode os.FileMode) (uint32, error) {
+	sysMode := uint32(mode.Perm())
+	if mode&os.ModeSetuid != 0 {
+		sysMode |= unix.S_ISUID
+	}
+	if mode&os.ModeSetgid != 0 {
+		sysMode |= unix.S_ISGID
+	}
+	if mode&os.ModeSticky != 0 {
+		sysMode |= unix.S_ISVTX
+	}
+	// We don't allow file type bits.
+	if mode&os.ModeType != 0 {
+		return 0, fmt.Errorf("%w %+.3o (%s): type bits not permitted", errInvalidMode, mode, mode)
+	}
+	// We don't allow other unknown modes.
+	if mode&^modePermExt != 0 || sysMode&unix.S_IFMT != 0 {
+		return 0, fmt.Errorf("%w %+.3o (%s): unknown mode bits", errInvalidMode, mode, mode)
+	}
+	return sysMode, nil
+}
+
 // MkdirAllHandle is equivalent to [MkdirAll], except that it is safer to use
 // in two respects:
 //
@@ -39,17 +66,17 @@ var (
 // a brand new lookup of unsafePath (such as with [SecureJoin] or openat2) after
 // doing [MkdirAll]. If you intend to open the directory after creating it, you
 // should use MkdirAllHandle.
-func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err error) {
-	// Make sure there are no os.FileMode bits set.
-	if mode&^0o7777 != 0 {
-		return nil, fmt.Errorf("%w for mkdir 0o%.3o", errInvalidMode, mode)
+func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.File, Err error) {
+	unixMode, err := toUnixMode(mode)
+	if err != nil {
+		return nil, err
 	}
 	// On Linux, mkdirat(2) (and os.Mkdir) silently ignore the suid and sgid
 	// bits. We could also silently ignore them but since we have very few
 	// users it seems more prudent to return an error so users notice that
 	// these bits will not be set.
-	if mode&^0o1777 != 0 {
-		return nil, fmt.Errorf("%w for mkdir 0o%.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
+	if unixMode&^0o1777 != 0 {
+		return nil, fmt.Errorf("%w for mkdir %+.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
 	}
 
 	// Try to open as much of the path as possible.
@@ -104,9 +131,6 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		return nil, fmt.Errorf("%w: yet-to-be-created path %q contains '..' components", unix.ENOENT, remainingPath)
 	}
 
-	// Make sure the mode doesn't have any type bits.
-	mode &^= unix.S_IFMT
-
 	// Create the remaining components.
 	for _, part := range remainingParts {
 		switch part {
@@ -123,7 +147,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		// 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) {
+		if err := unix.Mkdirat(int(currentDir.Fd()), part, unixMode); 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 deadErr := isDeadInode(currentDir); deadErr != nil {
@@ -196,10 +220,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 // If you plan to open the directory after you have created it or want to use
 // an open directory handle as the root, you should use [MkdirAllHandle] instead.
 // This function is a wrapper around [MkdirAllHandle].
-//
-// NOTE: The mode argument must be set the unix mode bits (unix.S_I...), not
-// the Go generic mode bits ([os.FileMode]...).
-func MkdirAll(root, unsafePath string, mode int) error {
+func MkdirAll(root, unsafePath string, mode os.FileMode) error {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {
 		return err

mkdir_linux_test.go

@@ -20,14 +20,14 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode int) error
+type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode os.FileMode) error
 
-var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error {
+var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error {
 	// We can't check expectedPath here.
 	return MkdirAll(root, unsafePath, mode)
 }
 
-var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error {
+var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error {
 	// Same logic as MkdirAll.
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {
@@ -55,7 +55,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s
 	return nil
 }
 
-func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) {
+func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode os.FileMode, expectedMode int, expectedErr error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	require.NoError(t, err)
 	defer rootDir.Close()
@@ -248,31 +248,36 @@ func TestMkdirAllHandle_AsRoot(t *testing.T) {
 
 func testMkdirAll_InvalidMode(t *testing.T, mkdirAll mkdirAllFunc) {
 	for _, test := range []struct {
-		mode        int
+		mode        os.FileMode
 		expectedErr error
 	}{
-		// os.FileMode bits are invalid.
-		{int(os.ModeDir | 0o777), errInvalidMode},
-		{int(os.ModeSticky | 0o777), errInvalidMode},
-		{int(os.ModeIrregular | 0o777), errInvalidMode},
+		// unix.S_IS* bits are invalid.
+		{unix.S_ISUID | 0o777, errInvalidMode},
+		{unix.S_ISGID | 0o777, errInvalidMode},
+		{unix.S_ISVTX | 0o777, errInvalidMode},
+		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode},
 		// unix.S_IFMT bits are also invalid.
 		{unix.S_IFDIR | 0o777, errInvalidMode},
 		{unix.S_IFREG | 0o777, errInvalidMode},
 		{unix.S_IFIFO | 0o777, errInvalidMode},
+		// os.FileType bits are also invalid.
+		{os.ModeDir | 0o777, errInvalidMode},
+		{os.ModeNamedPipe | 0o777, errInvalidMode},
+		{os.ModeIrregular | 0o777, errInvalidMode},
 		// suid/sgid bits are silently ignored by mkdirat and so we return an
 		// error explicitly.
-		{unix.S_ISUID | 0o777, errInvalidMode},
-		{unix.S_ISGID | 0o777, errInvalidMode},
-		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode},
+		{os.ModeSetuid | 0o777, errInvalidMode},
+		{os.ModeSetgid | 0o777, errInvalidMode},
+		{os.ModeSetuid | os.ModeSetgid | os.ModeSticky | 0o777, errInvalidMode},
 		// Proper sticky bit should work.
-		{unix.S_ISVTX | 0o777, nil},
+		{os.ModeSticky | 0o777, nil},
 		// Regular mode bits.
 		{0o777, nil},
 		{0o711, nil},
 	} {
 		root := t.TempDir()
 		err := mkdirAll(t, root, "a/b/c", test.mode)
-		assert.ErrorIsf(t, err, test.expectedErr, "mkdirall 0o%.3o", test.mode)
+		assert.ErrorIsf(t, err, test.expectedErr, "mkdirall %+.3o (%s)", test.mode, test.mode)
 	}
 }
 
@@ -295,7 +300,7 @@ func newRacingMkdirMeta() *racingMkdirMeta {
 	}
 }
 
-func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode int, allowedErrs []error) {
+func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode os.FileMode, allowedErrs []error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	require.NoError(t, err, "open root")
 	defer rootDir.Close()