Repository: diskfs/go-diskfs
Pull request #199 (2023-12-16 17:37:43)

squashfs: set Mode bits correctly in FileInfo

deitch: I have gone through some very not fun times dealing with when this should have the dir, etc. mode bits set and when not. Are you sure that this is consistently handled this way in go? i.e. when I use the usual built-in `os` commands to get `FileInfo`, does it always return with these bits set? If so, I am fine with it, but I want to be sure.
ncw: > I have gone through some very not fun times dealing with when this should have the dir, etc. mode bits set and when not.

Yes I've been there too :-)

> Are you sure that this is consistently handled this way in go? i.e. when I use the usual built-in `os` commands to get `FileInfo`, does it always return with these bits set? If so, I am fine with it, but I want to be sure.

The `os.FileMode` has had a chequered history in Go. At first it was just whatever was read from the OS but then Go got Windows support which is like putting a round peg in a square hole so it had to be changed. It is still rather unix centric, but it is platform independent now.

If you look at the help for `fs.FileMode` (`os.FileMode` is just an alias now-a-days) you'll see

```go
type FileMode uint32
    A FileMode represents a file's mode and permission bits. The bits have the
    same definition on all systems, so that information about files can be moved
    from one system to another portably. Not all bits apply to all systems.
    The only required bit is ModeDir for directories.
```

And if you look at [the actual definition of the bits](https://github.com/golang/go/blob/9b4b3e5acca2dabe107fa2c3ed963097d78a4562/src/io/fs/fs.go#L167-L193) you'll see that they are defined like this for all OSes.

```go
// The defined file mode bits are the most significant bits of the FileMode.
// The nine least-significant bits are the standard Unix rwxrwxrwx permissions.
// The values of these bits should be considered part of the public API and
// may be used in wire protocols or disk representations: they must not be
// changed, although new bits might be added.
const (
	// The single letters are the abbreviations
	// used by the String method's formatting.
	ModeDir        FileMode = 1 << (32 - 1 - iota) // d: is a directory
	ModeAppend                                     // a: append-only
	ModeExclusive                                  // l: exclusive use
	ModeTemporary                                  // T: temporary file; Plan 9 only
	ModeSymlink                                    // L: symbolic link
	ModeDevice                                     // D: device file
	ModeNamedPipe                                  // p: named pipe (FIFO)
	ModeSocket                                     // S: Unix domain socket
	ModeSetuid                                     // u: setuid
	ModeSetgid                                     // g: setgid
	ModeCharDevice                                 // c: Unix character device, when ModeDevice is set
	ModeSticky                                     // t: sticky
	ModeIrregular                                  // ?: non-regular file; nothing else is known about this file

	// Mask for the type bits. For regular files, none will be set.
	ModeType = ModeDir | ModeSymlink | ModeNamedPipe | ModeSocket | ModeDevice | ModeCharDevice | ModeIrregular

	ModePerm FileMode = 0777 // Unix permission bits
)
```

So I feel confident in saying that yes, it will always use exactly these bits whatever OS you are running on.

Perhaps the code I wrote to implement this should zero out everything except the 9 unix permissions bits so there aren't any Linux bits left in the mode so we've entirely Go-ified the mode. Currently it only zeroes out the `fs.ModeType` which means it should work for all Go programs but there is a possibility of reading any other bits from the mode.
deitch: > Perhaps the code I wrote to implement this should zero out everything except the 9 unix permissions bits so there aren't any Linux bits left in the mode so we've entirely Go-ified the mode.

Any downside to doing this? It sounds right to me, but 🤷‍♂️ 

> If you look at the help for fs.FileMode (os.FileMode is just an alias now-a-days) you'll see

Yeah, I implemented a read-write `fs.FS` interface and implementation for another project. Hard but good.
ncw: > > Perhaps the code I wrote to implement this should zero out everything except the 9 unix permissions bits so there aren't any Linux bits left in the mode so we've entirely Go-ified the mode.
> 
> Any downside to doing this? It sounds right to me, but 🤷‍♂️

I think making the FileMode 100% Go friendly is the right thing to do. If there are other bits in there we can extract to Go standard bits then that can be future work.

I've updated the PR

> Yeah, I implemented a read-write `fs.FS` interface and implementation for another project. Hard but good.

:-)




Patch:
diff --git a/filesystem/squashfs/directoryentry.go b/filesystem/squashfs/directoryentry.go
index cc5beb7..44e1aa9 100644
--- a/filesystem/squashfs/directoryentry.go
+++ b/filesystem/squashfs/directoryentry.go
@@ -107,7 +107,46 @@ func (d *directoryEntry) ModTime() time.Time {
 
 // Mode FileMode     // file mode bits
 func (d *directoryEntry) Mode() os.FileMode {
-	return d.mode
+	mode := d.mode
+
+	// We need to adjust the Linux mode into a Go mode
+	// The bottom 3*3 bits are the traditional unix permissions.
+
+	// Clear the non permissions bits
+	mode &= os.ModePerm
+
+	if d.inode == nil {
+		return mode
+	}
+	switch d.inode.inodeType() {
+	case inodeBasicDirectory, inodeExtendedDirectory:
+		mode |= os.ModeDir // d: is a directory
+	case inodeBasicFile, inodeExtendedFile:
+		// zero mode
+	case inodeBasicSymlink, inodeExtendedSymlink:
+		mode |= os.ModeSymlink // L: symbolic link
+	case inodeBasicBlock, inodeExtendedBlock:
+		mode |= os.ModeDevice // D: device file
+	case inodeBasicChar, inodeExtendedChar:
+		mode |= os.ModeDevice     // D: device file
+		mode |= os.ModeCharDevice // c: Unix character device, when ModeDevice is set
+	case inodeBasicFifo, inodeExtendedFifo:
+		mode |= os.ModeNamedPipe // p: named pipe (FIFO)
+	case inodeBasicSocket, inodeExtendedSocket:
+		mode |= os.ModeSocket // S: Unix domain socket
+	default:
+		mode |= os.ModeIrregular // ?: non-regular file; nothing else is known about this file
+	}
+
+	// Not currently translated
+	// mode |= os.ModeAppend          // a: append-only
+	// mode |= os.ModeExclusive       // l: exclusive use
+	// mode |= os.ModeTemporary       // T: temporary file; Plan 9 only
+	// mode |= os.ModeSetuid          // u: setuid
+	// mode |= os.ModeSetgid          // g: setgid
+	// mode |= os.ModeSticky          // t: sticky
+
+	return mode
 }
 
 // Sys interface{}   // underlying data source (can return nil)


Test patch:
diff --git a/filesystem/squashfs/squashfs_test.go b/filesystem/squashfs/squashfs_test.go
index 90c5ef0..0051b24 100644
--- a/filesystem/squashfs/squashfs_test.go
+++ b/filesystem/squashfs/squashfs_test.go
@@ -349,6 +349,19 @@ func TestSquashfsCheckListing(t *testing.T) {
 			if fi.IsDir() {
 				list(p)
 			}
+			// Check the type
+			var wantMode = os.FileMode(0)
+			if fi.IsDir() {
+				wantMode |= os.ModeDir
+			}
+			switch p {
+			case "/symlink", "/goodlink", "/emptylink":
+				wantMode |= os.ModeSymlink
+			}
+			gotMode := fi.Mode()
+			if (gotMode & os.ModeType) != wantMode {
+				t.Errorf("%s: want mode 0o%o got mode 0o%o", p, wantMode, gotMode&os.ModeType)
+			}
 		}
 	}