Respect umask in WriteFile

[hansmi]

Fixes #33 by taking the umask into consideration in renameio.WriteFile.

[twpayne]

Just checking that this won't change the current behavior for existing users of renameio.WriteFile. My specific case:

• I use renameio.WriteFile.
• I handle umask already, including allowing the user to set a my-application-specific umask, which may be more permissive than the system's umask.

This PR is clearly an improvement. If this PR does change the default behavior of renameio.WriteFlie then it should be accompanied by a major version bump in the next release of renameio.

[hansmi]

I handle umask already, including allowing the user to set a my-application-specific umask, which may be more permissive than the system's umask.

I'm assuming you don't set the application-specific umask via umask(2) (e.g. because it may vary per user and goroutine). With this PR there will be a change in behaviour depending on what the system/process umask is. That umask will be applied on top of your permissions, regardless of whether you applied an application umask or not—the syscall just sees the permissions given to open(2). Before this PR the file permissions would always be overwritten using fchmod(2) and that's where renameio.WriteFile differed from ioutil.WriteFile (see #33).

So yes, as you suggest, a major version bump is in order. @stapelberg, when reviewing and merging, can you tag a new release?

[twpayne]

I'm assuming you don't set the application-specific umask via umask(2)

You're correct :) I use the umask at startup as the default value, but the user can override it in their config file. The "umask at startup" is read in a package's init function, and I rely on hope that no other package modifies it beforehand.

Random other umask factoids that I discovered the hard way:

• Debian-based and RedHat-based Linux distributions use different default umasks (you already knew this).
• Most distributions created new users' home directories by copying /etc/skel and the files in /etc/skel are not always consistent with that distribution's umask (specifically, group permissions can differ).
• There is a bug in go test: it re-uses cached test results even if the umask has changed, which is incorrect. Specifically, umask 002 ; go test ./... ; umask 022 ; go test ./... should re-run the tests on the second invocation of go test ./... but instead go test re-uses the cached test results (I haven't reported this because I haven't dived deep enough to provide a fix along with the bug report).

[stapelberg]

While I appreciate proceeding with caution, I’m not sure we really need a major version bump here?

Citing from https://golang.org/doc/modules/version-numbers:

A module developer should increment this number past v1 only when necessary because the version upgrade represents significant disruption for developers whose code uses function in the upgraded module. This disruption includes backward-incompatible changes to the public API, as well as the need for developers using the module to update the package path wherever they import packages from the module.

I’d like to avoid everyone having to switch their imports to /v2, because I suspect that few people actually care about the umask.

Do we have any indication that this change might break existing setups? Can we identify repositories that use both renameio and umask handling?

Or, asked differently, what guidance would we give developers who are upgrading from the current version to the new version? How should they change their code, and/or verify things are working?

[twpayne]

Do we have any indication that this change might break existing setups? Can we identify repositories that use both renameio and umask handling?

I think it will break chezmoi in some edge cases. google/renameio is imported by at least 64 importers, which in turn may be imported themselves. I don't think it's reasonable to audit so many packages for dependence on the current behavior.

Or, asked differently, what guidance would we give developers who are upgrading from the current version to the new version? How should they change their code, and/or verify things are working?

Maybe something like:

"Since version X, renameio.WriteFile changes the way that permissions are handled. Before version X, the file was created with the permissions passed to the function. From version X onwards, these permissions are further modified by the user's umask. If you were relying on the pre-version X behavior, you must change your code to explicitly set the permissions you want after calling renameio.WriteFile, for example with os.Chmod. Note that the call to os.Chmod will not be part of the atomic replacement, i.e. the file may be read, modified, or deleted between the calls to renameio.WriteFile and os.Chmod."

I actually want the current behavior and I want the precise permission setting to be part of the atomic operation, and it is not possible to achieve the current behavior with the change proposed here. Some options:

1. Modify the renameio.WriteFile to allow the user to pass in renameio.Options which are then passed to NewPendingFile.
2. Add a new function that behaves as the current renameio.WriteFile for users who want the current behavior, although they will have to modify their code to use the new function.
3. Bump the version of this repo to v2 when this PR is merged.
4. Tell users to use a fork of this repo instead of google/renameio.
5. Tell users to exclude all versions of google/renameio that implement the new behavior in their go.mod files (this only works for the main module of course, users will have to audit their go.mod to see if they actually depend on google/renameio).

Of these, my preference order is 3, 2, 1, 4, 5.

I’d like to avoid everyone having to switch their imports to /v2, because I suspect that few people actually care about the umask.

Bumping major versions of Go packages really isn't a big deal. https://github.com/google/go-github is already a v39, for example. Major version bumps are for exactly these sort of changes. Silently changing the default behavior without providing a work-around for users to even achieve the current behavior is pretty hostile.

[stapelberg]

Thanks for the details.

I agree that bumping the major version isn’t that big a deal, but it also sounds like there are other reasons to change this PR to be backwards-compatible (keep current behavior), probably by introducing an optional options struct.

[hansmi]

I'd like to have a confirmation before I touch the code again.

1. WriteFile is documented as follows:

   WriteFile mirrors ioutil.WriteFile, replacing an existing file with the same name atomically

   We should clarify that WriteFile doesn't mirror ioutil.WriteFile fully, but rather always sets the mode as given in the perm parameter ignoring the umask.

2. Add another function receiving options, e.g. WriteFileWithOptions(path string, data []byte, opts ...Option) error. Users can then invoke that with WithPermissions(perm) to have the umask respected.

Is this correct?

[twpayne]

My $0.02c:

1. We should make renameio.WriteFile behave like ioutil.WriteFile by default, i.e. respect the umask.
2. We should add opts ...Option to renameio.WriteFile so WithPermissions(perm) can be passed as @hansmi proposes above. Note that this does not change the use of renameio.WriteFile for existing users.
3. We should bump google/renameio to v2 to reflect the change in default behavior.

[hansmi]

@stapelberg, do you have an opinion on which path we should take?

[stapelberg]

Thanks for the details. I would prefer:

1. Make renameio.WriteFile mirror ioutil.WriteFile, because I consider that the cleaner long-term goal. The shorter the docs can be (without any extra thought that users need to spend on whether the umask affects them), the better.
2. We can add opts ...Option to renameio.WriteFile (no need for a WriteFileWithOptions), because we need to bump the major version anyway.
3. We should document (in README.md, I suppose) how to switch from the current version to the v2 version. Maybe something along the lines of (adapting @twpayne’s suggestion):

With major version renameio/v2, renameio.WriteFile changes the way that permissions are handled. Before version 2, files were created with the permissions passed to the function, ignoring the umask. From version 2 onwards, these permissions are further modified by the user's umask.

If you were relying on the umask being ignored, add the renameio.IgnoreUmask() option to your renameio.WriteFile calls when upgrading to v2.

[hansmi]

@stapelberg, @twpayne, the latest revision of my changes include the IgnoreUmask option mentioned in https://github.com/google/renameio/pull/36#issuecomment-929102354 and update the README file. Please take another look.

[twpayne]

This looks great @hansmi. Thank you so much for your work and patience!