fdio: Use O_TMPFILE + rename-overwrite for regfile copies #80
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I was working on rpm-ostree unified core, and hit the fact that `glnx_file_copy_at()` had the same bug with `fsetxattr()` and files whose mode is <= `0400` (e.g. `000` in the case of `/etc/shadow`) that libostree did a while ago. This situation is masked for privileged (i.e. `CAP_DAC_OVERRIDE`) code. Looking at this, I think it's cleaner to convert to `O_TMPFILE` here, since that code already handles setting the tmpfile to mode `0600`. Now, this *is* a behavior change in the corner case of existing files which are symbolic links. Previously we'd do an `open(O_TRUNC)` which would follow the link. But in the big picture, I think the use cases for `open(O_TRUNC)` are really rare - I audited all callers of this in ostree/rpm-ostree/flatpak, and all of them will be fine with this behavior change. For example, the ostree `/etc` merge code already explicitly unlinks the target beforehand. Other cases like supporting `repo/pubring.gpg` in an ostree repo being a symlink...eh, just no. Making this change allows us to convert to new style, and brings all of the general benefits of using `O_TMPFILE` too.
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Sep 11, 2017
Prep for unified core work. This was failing due to a bug in libglnx <GNOME/libglnx#80> but I think this change is also correct. There's no good reason for us to copy xattrs like the SELinux label here - rather we want the labels to be reset during commit.
jlebon
reviewed
Sep 12, 2017
| @@ -858,11 +858,15 @@ glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes) | |||
| * @cancellable: cancellable | |||
There was a problem hiding this comment.
Nit/not new to this patch: the src_stbuf parameter isn't in the docstring.
| } | ||
|
|
||
| /* Always chmod after setting xattrs, in case the file has mode 0400 or less, | ||
| * like /etc/shadow. |
There was a problem hiding this comment.
I think I'm missing something here. Does setting xattrs reset the mode? Can you link to the ostree issue in the commit message or the comment?
There was a problem hiding this comment.
Basically if a file is mode 000 (or in general doesn't have write perms), one can write() to it but not fsetxattr(). ostreedev/ostree@3348baf is related and so is 452c371
I thought there was a better commit but I can't find it.
There was a problem hiding this comment.
In other words we need to fchmod() after we do the xattrs, and the file needs to be writable before that, which our O_TMPFILE code ensures.
There was a problem hiding this comment.
Ahh gotcha, wasn't aware of that. Maybe make it more explicit in the comment? Otherwise, LGTM!
|
Updated for both issues and pushed, thanks! |
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Sep 25, 2017
Prep for unified core work. This was failing due to a bug in libglnx <GNOME/libglnx#80> but I think this change is also correct. There's no good reason for us to copy xattrs like the SELinux label here - rather we want the labels to be reset during commit.
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Sep 25, 2017
Prep for unified core work. This was failing due to a bug in libglnx <GNOME/libglnx#80> but I think this change is also correct. There's no good reason for us to copy xattrs like the SELinux label here - rather we want the labels to be reset during commit. I did a tree-wide grep for other users and the only other case that is odd is the treecompose `add-files`; I'd say we should change this but out of (a likely excess of) conservatism I just left a "FIXME" for now.
rh-atomic-bot
pushed a commit
to coreos/rpm-ostree
that referenced
this pull request
Sep 26, 2017
Prep for unified core work. This was failing due to a bug in libglnx <GNOME/libglnx#80> but I think this change is also correct. There's no good reason for us to copy xattrs like the SELinux label here - rather we want the labels to be reset during commit. I did a tree-wide grep for other users and the only other case that is odd is the treecompose `add-files`; I'd say we should change this but out of (a likely excess of) conservatism I just left a "FIXME" for now. Closes: #1008 Approved by: jlebon
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was working on rpm-ostree unified core, and hit the fact that
glnx_file_copy_at()had the same bug withfsetxattr()and files whose modeis <=
0400(e.g.000in the case of/etc/shadow) that libostree did awhile ago. This situation is masked for privileged (i.e.
CAP_DAC_OVERRIDE)code.
Looking at this, I think it's cleaner to convert to
O_TMPFILEhere,since that code already handles setting the tmpfile to mode
0600. Now,this is a behavior change in the corner case of existing files which
are symbolic links. Previously we'd do an
open(O_TRUNC)which would followthe link.
But in the big picture, I think the use cases for
open(O_TRUNC)are reallyrare - I audited all callers of this in ostree/rpm-ostree/flatpak, and all of
them will be fine with this behavior change. For example, the ostree
/etcmerge code already explicitly unlinks the target beforehand. Other cases like
supporting
repo/pubring.gpgin an ostree repo being a symlink...eh, just no.Making this change allows us to convert to new style, and brings all of the
general benefits of using
O_TMPFILEtoo.