Skip to content

Return only the type bits from iso9660 DirEntry.Type - #432

Open
youdie006 wants to merge 1 commit into
diskfs:masterfrom
youdie006:iso9660-direntry-type-bits
Open

Return only the type bits from iso9660 DirEntry.Type#432
youdie006 wants to merge 1 commit into
diskfs:masterfrom
youdie006:iso9660-direntry-type-bits

Conversation

@youdie006

Copy link
Copy Markdown

iso9660's DirEntry.Type() returns the full mode instead of the type bits, so a caller comparing
against fs.ModeDir never matches and reads directories as regular files.

io/fs, fs.go:112:

Type returns the type bits for the entry. The type bits are a subset of the usual FileMode bits,
those returned by the FileMode.Type method.

filesystem/iso9660/directoryentry.go:598:

func (de *directoryEntry) Type() os.FileMode {
	return de.Mode()
}

de.Mode() returns os.ModeDir | 0o755, os.ModeSymlink | 0o777, 0o755, or the Rock Ridge POSIX
mode — permissions included.

Its three siblings already do it right

squashfs/directoryentry.go:77 return d.Mode().Type()
fat12/directoryentry.go:84 iofs.ModeDir / 0
ext4/directoryentry.go:210 type constants only
iso9660/directoryentry.go:599 return de.Mode()

What it costs

testing/fstest.TestFS on the committed testdata/9660.iso:

ABC: mismatch:
	entry = ABC IsDir=true Type=drwxr-xr-x
	file.Stat() = ABC IsDir=true Type=d---------
ABC: Open+ReadAll: invalid argument
BAR: mismatch:
	entry = BAR IsDir=true Type=drwxr-xr-x
	file.Stat() = BAR IsDir=true Type=d---------
BAR: Open+ReadAll: invalid argument
DEEP: mismatch:

Five mismatches, none after the change. The Open+ReadAll: invalid argument lines are the real
cost: fstest dispatches on info.Type() ($(go env GOROOT)/src/testing/fstest/testfs.go:159),
and because drwxr-xr-x is not fs.ModeDir every subdirectory falls to the default branch and is
opened 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.ModeSymlink to spot a Rock Ridge symlink, hits the same wall. fs.WalkDir survives only
because 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:

// iofs.DirEntry.Type returns only the type bits, not the permissions
if typ := e.Type(); typ != typ.Type() {
    t.Fatalf("entry %q in %q: Type() = %v, want only type bits %v", name, path, typ, typ.Type())
}

Red with only directoryentry.go reverted — and only iso9660 fails:

fsvalidate.go:37: entry "ABC" in ".": Type() = drwxr-xr-x, want only type bits d---------

Green with the change. go test ./filesystem/... passes across all seven packages (ext4 included,
115s), gofmt -l lists neither file, go vet is clean.

This is the shape of #357 ("fix(iso9660): Open() now handles directories and implements
fs.ReadDirFile"), which is also where TestFSTree came from.

One thing I found and did not fix

With this change in place, fstest.TestFS surfaces a second, unrelated violation: a path containing
a backslash resolves to a real entry. Open("DEEP\\A") correctly reports "file does not exist", but
Open("DEEP\\A/B") succeeds — the path-table lookup at filesystem/iso9660/pathtable.go:152 misses
and falls through to rootDir.getLocation, which accepts the bogus component. Separate PR; happy to
send it if useful.


Disclosure: prepared with AI assistance; I verified the fstest.TestFS output, the sibling
comparison and the red/green runs myself.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant