rc.init: fix mkchar not setting correct filemode #3624
Conversation
|
@djs55 @justincormack PTAL |
| @@ -55,13 +55,15 @@ func mountSilent(source string, target string, fstype string, flags uintptr, dat | |||
| func mkchar(path string, mode, major, minor uint32) { | |||
| // unix.Mknod only supports int dev numbers; this is ok for us | |||
| dev := int(unix.Mkdev(major, minor)) | |||
| err := unix.Mknod(path, mode, dev) | |||
| err := unix.Mknod(path, mode|unix.S_IFCHR, dev) | |||
| if err != nil { | |||
| if err.Error() == "file exists" { | |||
| return | |||
There was a problem hiding this comment.
Do we want to chmod in all cases (also in this case?) or only if we actually created?
There was a problem hiding this comment.
I would rather set the umask at the beginning of the code so the permissions are unaffected and then set it to 022 at the end for the children to inherit, rather than also calling chmod all over the place.
There was a problem hiding this comment.
Yes, I was doubting about that; I know we had/have some code in docker that changes umask, and it caused issues for processes that were running concurrently.
But I guess that's not a problem for this case (no other code should be running concurrently 😅)
|
Could you |
|
oh! yes, looks like I didn't enable my |
|
👍 should be fixed now |
Commit bcfb760 rewrote the init from a shell script to a go implementation. However, unlike `mknod`, the active umask is applied when using Go's unix.Mknod(). This patch: - sets the correct mode when calling unix.Mknod() - temporarily overrides the umask during doMounts() Steps I used to reproduce the original issue, and to verify the changes in this patch (tried inside a container): docker run -it --rm -w /app golang bash cat > mknod.go <<'EOF' package main import ( "log" "golang.org/x/sys/unix" ) func main() { mkchar("/dev/null2", 0666, 1, 3) umask := unix.Umask(0000) defer unix.Umask(umask) mkchar2("/dev/null3", 0666, 1, 3) } // make a character device func mkchar(path string, mode, major, minor uint32) { // unix.Mknod only supports int dev numbers; this is ok for us dev := int(unix.Mkdev(major, minor)) err := unix.Mknod(path, mode, dev) if err != nil { if err.Error() == "file exists" { return } log.Printf("error making device %s: %v", path, err) } } // make a character device func mkchar2(path string, mode, major, minor uint32) { // unix.Mknod only supports int dev numbers; this is ok for us dev := int(unix.Mkdev(major, minor)) err := unix.Mknod(path, mode|unix.S_IFCHR, dev) if err != nil { if err.Error() == "file exists" { return } log.Printf("error making device %s: %v", path, err) } } EOF Initialize module and fetch dependencies: go mod init foo && go mod tidy Check active umask: umask 0022 Run the test code: go run mknod.go Check the results: ls -la /dev/null* crw-rw-rw- 1 root root 1, 3 Apr 13 11:45 /dev/null -rw-r--r-- 1 root root 0 Apr 13 11:45 /dev/null2 crw-rw-rw- 1 root root 1, 3 Apr 13 11:45 /dev/null3 Notice that: - `/dev/null2` (before) was created with active umask (`0022`) applied, and did not create a character device - `/dev/null3` (after) has both the correct (0666) permissions and mode Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
@djs55 @justincormack updated; PTAL |
|
I've been playing with this image inside Docker Desktop. The permissions are still not what I would expect :( I added some debug: diff --git a/pkg/init/cmd/rc.init/main.go b/pkg/init/cmd/rc.init/main.go
index caba3a666..bcae5e248 100644
--- a/pkg/init/cmd/rc.init/main.go
+++ b/pkg/init/cmd/rc.init/main.go
@@ -54,16 +54,26 @@ func mountSilent(source string, target string, fstype string, flags uintptr, dat
// make a character device
func mkchar(path string, mode, major, minor uint32) {
// unix.Mknod only supports int dev numbers; this is ok for us
+ log.Printf("mkchar %s %o", path, mode)
dev := int(unix.Mkdev(major, minor))
err := unix.Mknod(path, mode|unix.S_IFCHR, dev)
+ // make sure we get the correct filemode, irregardless of active umask
+ err = os.Chmod(path, os.FileMode(mode)|unix.S_IFCHR)
+ log.Printf("chmod %s %o err = %v", path, mode, err)
if err != nil {
if err.Error() == "file exists" {
+ log.Printf("file exists")
return
}
log.Printf("error making device %s: %v", path, err)
}
- // make sure we get the correct filemode, irregardless of active umask
- _ = os.Chmod(path, os.FileMode(mode)|unix.S_IFCHR)
+ if stat, err := os.Stat(path); err == nil {
+ log.Printf("after Mknod %s has mode %o", path, stat.Mode())
+ }
+
+ if stat, err := os.Stat(path); err == nil {
+ log.Printf("after Chmod %s has mode %o", path, stat.Mode())
+ }
}
// symlink with error warningand the logging looks correct: but I still see I need to debug more. |
|
Hm if I comment out the doHotplug function then it works: |
|
The permissions seem to change after running
|
|
What's odd is that even the older bash style init script called |
|
Any progress on this ? @thaJeztah
Are we sure that the behavior was correct with older bash style init ? It looks to me that |
Commit bcfb760 (#2500) rewrote the init from a shell script to a go implementation. However, unlike
mknod, the active umask is applied when using Go's unix.Mknod().This patch:
Steps I used to reproduce the original issue, and to verify the changes in this
patch (tried inside a container):
Initialize module and fetch dependencies:
Check active umask:
Run the test code:
Check the results:
Notice that:
/dev/null2(before) was created with active umask (0022) applied, and did not create a character device/dev/null3(after) has both the correct (0666) permissions and mode- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)