Skip to content

testing/fstest: set perm 0555 for synthetized directories in MapFS #63468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dolmen opened this issue Oct 9, 2023 · 6 comments
Closed

testing/fstest: set perm 0555 for synthetized directories in MapFS #63468

dolmen opened this issue Oct 9, 2023 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dolmen
Copy link
Contributor

dolmen commented Oct 9, 2023

testing/fstest.MapFS should set a FileMode compatible with its behaviour: as reading and traversing directories just ignores filemodes (it doesn't check filemode before giving access to file content or directory content), it would be more consistent to synthetise directories with mode fs.ModeDir | 0555 instead of fs.ModeDir | 0000.

What version of Go are you using (go version)?

$ go version
go version go1.21.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://go.dev/play/p/3_nH4WnurYA

func Test(t *testing.T) {
	m := fstest.MapFS{
		"subdir/a.txt": &fstest.MapFile{
			Mode: 0444,
		},
		"subdir/subdir2/a.txt": &fstest.MapFile{
			Mode: 0444,
		},
	}

	info, err := fs.Stat(m, "subdir")
	if err != nil {
		t.Fatal(err)
	}
	t.Logf("%o %[1]s %s", info.Mode(), info.Name())

	info, err = fs.Stat(m, "subdir/subdir2")
	if err != nil {
		t.Fatal(err)
	}
	t.Logf("%o %[1]s %s", info.Mode(), info.Name())

	// t.Logf("%o %[1]s", info.Mode() | 0555)
}

What did you expect to see?

=== RUN   Test
    prog_test.go:23: 20000000555 dr-xr-xr-x subdir
    prog_test.go:29: 20000000555 dr-xr-xr-x subdir2
--- PASS: Test (0.00s)
PASS

What did you see instead?

=== RUN   Test
    prog_test.go:23: 20000000000 d--------- subdir
    prog_test.go:29: 20000000000 d--------- subdir2
--- PASS: Test (0.00s)
PASS
@dolmen
Copy link
Contributor Author

dolmen commented Oct 9, 2023

Fix:

diff --git a/src/testing/fstest/mapfs.go b/src/testing/fstest/mapfs.go
index a0b1f65668..b3fc0c8ad7 100644
--- a/src/testing/fstest/mapfs.go
+++ b/src/testing/fstest/mapfs.go
@@ -98,14 +98,14 @@ func (fsys MapFS) Open(name string) (fs.File, error) {
                delete(need, fi.name)
        }
        for name := range need {
-               list = append(list, mapFileInfo{name, &MapFile{Mode: fs.ModeDir}})
+               list = append(list, mapFileInfo{name, &MapFile{Mode: fs.ModeDir | 0555}})
        }
        sort.Slice(list, func(i, j int) bool {
                return list[i].name < list[j].name
        })
 
        if file == nil {
-               file = &MapFile{Mode: fs.ModeDir}
+               file = &MapFile{Mode: fs.ModeDir | 0555}
        }
        return &mapDir{name, mapFileInfo{elem, file}, list, 0}, nil
 }

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 9, 2023
@dmitshur dmitshur added this to the Backlog milestone Oct 9, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Oct 9, 2023

CC @bcmills, @ianlancetaylor.

@ianlancetaylor
Copy link
Member

Want to send a patch? https://go.dev/doc/contribute.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/534075 mentions this issue: testing/fstest: MapFS: set perm 0555 on synthetized dirs

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 11, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Oct 11, 2023
@dolmen
Copy link
Contributor Author

dolmen commented Oct 12, 2023

🎉

yunginnanet pushed a commit to yunginnanet/go that referenced this issue Oct 20, 2023

Verified

This commit was signed with the committer’s verified signature.
As MapFS ignores filemodes and always grant read and traverse access on
directories, let's make synthetized directory entries to expose filemode
0555 instead of 0000.
Fixes golang#63468.

Change-Id: I5d64a6bf2f2ac6082ca5dde55b3062669fb50b8d
Reviewed-on: https://go-review.googlesource.com/c/go/+/534075
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@jsageryd
Copy link
Contributor

While I do agree that it makes sense for directories to have 0555 set, this change actually broke my tests that relied on fstest.MapFS synthesizing directories with only mode fs.ModeDir. The exact behaviour of fstest.MapFS wasn't documented and I suppose one might even be able to call the previous behaviour a bug, so I don't think it breaks the Go 1 compatibility promise, and I have no issue with that part.

However, the documentation does still suggest that a directory can be created by setting the file mode to fs.ModeDir. This is true, but following this recommendation now results in a directory that differs from what fs.MapFS would synthesize by default. Would it make sense to update the documentation to recommend setting the mode to fs.ModeDir | 0555 (or perhaps fs.ModeDir | 0o555)?

The map need not include parent directories for files contained in the map; those will be synthesized if needed. But a directory can still be included by setting the [MapFile.Mode]'s fs.ModeDir bit; this may be necessary for detailed control over the directory's fs.FileInfo or to create an empty directory.

https://pkg.go.dev/testing/fstest

@golang golang locked and limited conversation to collaborators Feb 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants