Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WriteFile doesn't respect umask #33

Closed
hansmi opened this issue Aug 20, 2021 · 7 comments · Fixed by #36
Closed

WriteFile doesn't respect umask #33

hansmi opened this issue Aug 20, 2021 · 7 comments · Fixed by #36

Comments

@hansmi
Copy link
Contributor

hansmi commented Aug 20, 2021

On Unix systems the renameio.WriteFile function does not respect the umask(2). This is directly caused by ioutil.TempFile always setting the file mode to 0600, in contrast to ioutil.WriteFile which passes the desired mode to os.OpenFile where it's forwarded to the underlying syscall creating the file after applying the umask.

The active umask can't be looked up without changing it and, optionally, restoring the previous value (see syscall.Umask and man page linked above). This is no good when multiple file operations may be ongoing concurrently. As such I don't see an immediate solution without avoiding ioutil.TempFile.

Reproduction with a umask of 0077 (drop all permissions for the group and others):

$ umask 0077; go run umask.go /tmp/umask0077.txt; stat -c '%A %a %n' /tmp/umask0077.txt*
-rw------- 600 /tmp/umask0077.txt.ioutil
-rw-r--r-- 644 /tmp/umask0077.txt.renameio

Test code:

package main

import (
        "io/ioutil"
        "log"
        "os"

        "github.com/google/renameio"
)

func main() {
        if err := renameio.WriteFile(os.Args[1]+".renameio", nil, 0644); err != nil {
                log.Fatal(err)
        }

        if err := ioutil.WriteFile(os.Args[1]+".ioutil", nil, 0644); err != nil {
                log.Fatal(err)
        }
}
@stapelberg
Copy link
Collaborator

Thanks for the detailed report!

This is no good when multiple file operations may be ongoing concurrently. As such I don't see an immediate solution without avoiding ioutil.TempFile.

Do you see a solution with avoiding ioutil.TempFile? I can’t seem to find any entry point for creating a temporary file with permissions that respect the umask in Go…?

@hansmi
Copy link
Contributor Author

hansmi commented Aug 22, 2021

Do you see a solution with avoiding ioutil.TempFile?

For Linux 4.7 and newer the umask is exposed via procfs (commit; backported to older kernels by a few vendors):

$ grep ^Umask /proc/self/status
Umask:	0022

Unfortunately the only portable alternative I'm aware of is to reimplement ioutil.TempFile (no need for the pattern functionality). It's not the most complex function and at the same time it's annoying having to reimplement it. All we'd need is the ability to control the mode given to OpenFile (Go 1.17: https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/os/tempfile.go;l=44;drc=refs%2Ftags%2Fgo1.17).

@twpayne
Copy link
Contributor

twpayne commented Aug 23, 2021

Should the respect of umask be punted to the application code?

Any attempt to read the umask and then apply it when writing a new file will be inherently racy due to the delay between the read and the apply.

Surely applications should know their initial umask and also know when that umask changes?

@hansmi
Copy link
Contributor Author

hansmi commented Aug 23, 2021

Should the respect of umask be punted to the application code?
[…]
Surely applications should know their initial umask and also know when that umask changes?

The Go runtime would be able to determine the umask at start via the umask(2) system call, yes, but once mortal Go code runs it's impossible to do so safely: init() functions could already be touching the filesystem or launching goroutines doing the same. Remember that reading the umask in a portable manner requires resetting it—it's a non-atomic operation. Reading from procfs would be preferrable at that point, though non-portable.

Thus this responsibility can't be given to application code. Instead open(2) or an equivalent should be invoked for non-critical files with an acceptable set of permissions (often 0644) which will then be trimmed according to the umask configured by the user or system operator.

So yes, that leaves us with replacing ioutil.TempFile. @stapelberg, do you see another option?

@stapelberg
Copy link
Collaborator

So yes, that leaves us with replacing ioutil.TempFile. @stapelberg, do you see another option?

I don’t see any other option.

Would you be willing to send a PR which makes the code change? Thanks

@hansmi
Copy link
Contributor Author

hansmi commented Aug 24, 2021

Would you be willing to send a PR which makes the code change?

Yes, I'll prepare one.

@stapelberg
Copy link
Collaborator

Okay, I have added /v2 to the module name in go.mod and tagged+pushed v2.0.0.

Let me know if anything else needs to be done or changed.

Thanks very much for your contribution!

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 a pull request may close this issue.

3 participants