parsing os.FileMode doesn't check upper boundaries
prochac opened this issue · 5 comments
The parser ignores if parsed value is "overflowing" file mode range.
https://play.golang.org/p/XhOFdvb44WY
It can be used to setup the first column
I know about this, but I am not sure if it is an issue. If you look to Go documentation, you will notice that fs.FileMode is an alias to uint32 and the documentation states, that only least nine significant bits represents UNIX permissions. The rest is reserved for file mode bits and can have arbitrary meaning on different systems (as I understand). Thus, IMHO, it is perfectly fine that we do not check for overflow, and that the two permission strings you print in the play.golang.org example are the same.
I am open to discussion. What is your opinion?
Imagine, I have this app what just creates directories.
https://play.golang.org/p/bbN9t0_W8g-
What I want is to create a directory with similar behavior of /tmp. (everybody can add a file, but delete only their own)
So I have to create a directory with sticky bit, chmod +trwx or chmod 1777 on linux to achieve trwxrwxrwx.
The 1777 can't be used, because of the required zero prefix.
If I use "01777" then its parsed as -rwxrwxrwx. The true value is 1023 (0o1777), YES, but it doesn't work as intended, due to Go's internal handling of the value.
But, I can achieve my goal by using 04000777 as environment variable. And I hope you see that this doesn't feel right.
This is not env package related issue, but fs package related issue. We might have our opinion on the fs.FileMode implementation, but we have to kinda accept that. I believe you would agree that the env package cannot have domain knowledge of all possible types in all packages and treat them specially. Moreover, even if it could, the package shouldn't enforce its "strong opinion" on different type parsing. That's up to authors of types to implement proper serialization and deserialization for them.
The fact that env package parses fs.FileMode is mostly an accident (as it's uint32) than an intention. Naturally it parses it in a form which the fs.FileMode uses to represent file permissions. If you want to have a different behaviour, you can write your own type wrapping fs.FileMode and implementing encoding.TextUnmarshaler. This way, you will be able to have your own FileMode parsing exactly as you ask for. There is no need to change the behaviour for everyone else who could expect and even use the current parsing logic. That would be much more ideomatic solution, don't you think?
It's not accident. This PR adds support for os.FileMode.
#7
Partially, it feels like a weird support for octal numbers.
https://play.golang.org/p/mUtAFMIlYQH 😄
But my opinion has changed over the time this issue is open.
What I have expected before is support of Unix permissions. But they are not compatible with other platforms supported by Go.
Let's say I would like to create a temp file on Plan9.
https://pkg.go.dev/io/fs#ModeTemporary
So now the implementation, and configuration interface of imaginary application, is Go specific.
When I would like to do a rewrite of the imaginary app in Rust, I would need to mimic the Go behavior, or change the config interface.
There doesn't seem to be any win-win situation.
I guess the user should decide its way to go.
It's not accident. This PR adds support for os.FileMode.
You are right, sorry I have missed that there is separate parser. I expected it to be parsed as it'suint32.
I mostly agree with you. There is no win-win, as the parsing is platform specific and conventions differ platform per platform. We can in the future discuss the octal number parsing if necessary. I'm still not 100% certain that env package should support this parsing. For me the first choice would be encoding.TextUnmarshaler interface as there I'd be able to implement whichever logic my app needs.