Return only the type bits from iso9660 DirEntry.Type - #432
Open
youdie006 wants to merge 1 commit into
Open
Conversation
Type returned the full mode, permissions included, so a comparison against fs.ModeDir never matched and callers that switch on Type treated every directory as a regular file. squashfs, fat12 and ext4 already return type bits only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
iso9660'sDirEntry.Type()returns the full mode instead of the type bits, so a caller comparingagainst
fs.ModeDirnever matches and reads directories as regular files.io/fs,fs.go:112:filesystem/iso9660/directoryentry.go:598:de.Mode()returnsos.ModeDir | 0o755,os.ModeSymlink | 0o777,0o755, or the Rock Ridge POSIXmode — permissions included.
Its three siblings already do it right
squashfs/directoryentry.go:77return d.Mode().Type()fat12/directoryentry.go:84iofs.ModeDir/0ext4/directoryentry.go:210iso9660/directoryentry.go:599return de.Mode()What it costs
testing/fstest.TestFSon the committedtestdata/9660.iso:Five mismatches, none after the change. The
Open+ReadAll: invalid argumentlines are the realcost:
fstestdispatches oninfo.Type()($(go env GOROOT)/src/testing/fstest/testfs.go:159),and because
drwxr-xr-xis notfs.ModeDirevery subdirectory falls to thedefaultbranch and isopened as a file, so the walk never descends. The same run goes from 0.00s to 4.92s with the fix —
it is actually traversing the tree now. Any consumer switching on
DirEntry.Type(), or testing== fs.ModeSymlinkto spot a Rock Ridge symlink, hits the same wall.fs.WalkDirsurvives onlybecause it uses
IsDir().Change
One line. The check goes into the repo's own shared validator (
filesystem/internal/testutil/fsvalidate.go),which all four filesystem packages already run, so it guards every backend rather than just this one:
Red with only
directoryentry.goreverted — and only iso9660 fails:Green with the change.
go test ./filesystem/...passes across all seven packages (ext4 included,115s),
gofmt -llists neither file,go vetis clean.This is the shape of #357 ("fix(iso9660): Open() now handles directories and implements
fs.ReadDirFile"), which is also where
TestFSTreecame from.One thing I found and did not fix
With this change in place,
fstest.TestFSsurfaces a second, unrelated violation: a path containinga backslash resolves to a real entry.
Open("DEEP\\A")correctly reports "file does not exist", butOpen("DEEP\\A/B")succeeds — the path-table lookup atfilesystem/iso9660/pathtable.go:152missesand falls through to
rootDir.getLocation, which accepts the bogus component. Separate PR; happy tosend it if useful.
Disclosure: prepared with AI assistance; I verified the
fstest.TestFSoutput, the siblingcomparison and the red/green runs myself.