From de4951e4565a34a8bd5fd45d1beef3a09c3f282a Mon Sep 17 00:00:00 2001 From: Neha Arora Date: Mon, 16 Jun 2025 16:35:54 -0700 Subject: [PATCH 1/2] fix: only use the permission bits for chmod --- pkg/blob/blob.go | 8 +++-- pkg/blob/blob_test.go | 84 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/pkg/blob/blob.go b/pkg/blob/blob.go index b3735c9d2..46935a76c 100644 --- a/pkg/blob/blob.go +++ b/pkg/blob/blob.go @@ -1092,9 +1092,11 @@ func chmodIfPermissionMismatch(targetPath string, mode os.FileMode) error { return err } perm := info.Mode() & os.ModePerm - if perm != mode { - klog.V(2).Infof("chmod targetPath(%s, mode:0%o) with permissions(0%o)", targetPath, info.Mode(), mode) - if err := os.Chmod(targetPath, mode); err != nil { + expectedPerms := mode & os.ModePerm + if perm != expectedPerms { + klog.V(2).Infof("chmod targetPath(%s, mode:0%o) with permissions(0%o)", targetPath, info.Mode(), expectedPerms) + // only change the permission mode bits, keep the other bits as is + if err := os.Chmod(targetPath, (info.Mode()&^os.ModePerm)|os.FileMode(expectedPerms)); err != nil { return err } } else { diff --git a/pkg/blob/blob_test.go b/pkg/blob/blob_test.go index 2266e727d..23f67d88c 100644 --- a/pkg/blob/blob_test.go +++ b/pkg/blob/blob_test.go @@ -1516,11 +1516,23 @@ func TestChmodIfPermissionMismatch(t *testing.T) { _ = os.MkdirAll(permissionMismatchPath, os.FileMode(0721)) defer os.RemoveAll(permissionMismatchPath) + permissionMatchGidMismatchPath, _ := getWorkDirPath("permissionMatchGidMismatchPath") + _ = os.MkdirAll(permissionMatchGidMismatchPath, os.FileMode(0755)) + _ = os.Chmod(permissionMatchGidMismatchPath, 0755|os.ModeSetgid) // Setgid bit is set + defer os.RemoveAll(permissionMatchGidMismatchPath) + + permissionMismatchGidMismatch, _ := getWorkDirPath("permissionMismatchGidMismatch") + _ = os.MkdirAll(permissionMismatchGidMismatch, os.FileMode(0721)) + _ = os.Chmod(permissionMismatchGidMismatch, 0721|os.ModeSetgid) // Setgid bit is set + defer os.RemoveAll(permissionMismatchGidMismatch) + tests := []struct { - desc string - path string - mode os.FileMode - expectedError error + desc string + path string + mode os.FileMode + expectedPerms os.FileMode + expectedGidBit bool + expectedError error }{ { desc: "Invalid path", @@ -1529,16 +1541,52 @@ func TestChmodIfPermissionMismatch(t *testing.T) { expectedError: fmt.Errorf("CreateFile invalid-path: The system cannot find the file specified"), }, { - desc: "permission matching path", - path: permissionMatchingPath, - mode: 0755, - expectedError: nil, + desc: "permission matching path", + path: permissionMatchingPath, + mode: 0755, + expectedPerms: 0755, + expectedGidBit: false, + expectedError: nil, }, { - desc: "permission mismatch path", - path: permissionMismatchPath, - mode: 0755, - expectedError: nil, + desc: "permission mismatch path", + path: permissionMismatchPath, + mode: 0755, + expectedPerms: 0755, + expectedGidBit: false, + expectedError: nil, + }, + { + desc: "permission mismatch path", + path: permissionMismatchPath, + mode: 0755, + expectedPerms: 0755, + expectedGidBit: false, + expectedError: nil, + }, + { + desc: "only match the permission mode bits", + path: permissionMatchGidMismatchPath, + mode: 0755, + expectedPerms: 0755, + expectedGidBit: true, + expectedError: nil, + }, + { + desc: "only change the permission mode bits when gid is set", + path: permissionMismatchGidMismatch, + mode: 0755, + expectedPerms: 0755, + expectedGidBit: true, + expectedError: nil, + }, + { + desc: "only change the permission mode bits when gid is not set but mode bits have gid set", + path: permissionMismatchPath, + mode: 02755, + expectedPerms: 0755, + expectedGidBit: false, + expectedError: nil, }, } @@ -1549,7 +1597,19 @@ func TestChmodIfPermissionMismatch(t *testing.T) { t.Errorf("test[%s]: unexpected error: %v, expected error: %v", test.desc, err, test.expectedError) } } + + if test.expectedError == nil { + info, _ := os.Lstat(test.path) + if test.expectedError == nil && (info.Mode()&os.ModePerm != test.expectedPerms) { + t.Errorf("test[%s]: unexpected perms: %v, expected perms: %v, ", test.desc, info.Mode()&os.ModePerm, test.expectedPerms) + } + + if (info.Mode()&os.ModeSetgid != 0) != test.expectedGidBit { + t.Errorf("test[%s]: unexpected gid bit: %v, expected gid bit: %v", test.desc, info.Mode()&os.ModeSetgid != 0, test.expectedGidBit) + } + } } + } // getWorkDirPath returns the path to the current working directory From abc72b5d7f7bf03b74025a672acf343048711f39 Mon Sep 17 00:00:00 2001 From: Neha Arora Date: Tue, 17 Jun 2025 10:32:44 -0700 Subject: [PATCH 2/2] test: skip the TestChmodIfPermissionMismatch test for windows --- pkg/blob/blob_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/blob/blob_test.go b/pkg/blob/blob_test.go index 23f67d88c..e1ef9ee47 100644 --- a/pkg/blob/blob_test.go +++ b/pkg/blob/blob_test.go @@ -23,6 +23,7 @@ import ( "fmt" "os" "reflect" + "runtime" "sort" "strings" "testing" @@ -1508,6 +1509,10 @@ func TestIsSupportedContainerNamePrefix(t *testing.T) { } func TestChmodIfPermissionMismatch(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows") + } + permissionMatchingPath, _ := getWorkDirPath("permissionMatchingPath") _ = makeDir(permissionMatchingPath) defer os.RemoveAll(permissionMatchingPath)