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

postprocess: Ensure toplevel dirs are 0755 regardless of umask #1902

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

umask is one of those really evil Unix things...it's pretty
crazy actually there's still no threadsafe way to "mkdir ignoring umask".

This surfaced in someone using coreos-assembler with a working
directory of mode 0750 and having that surface in the target
rootfs.

Ref: coreos/fedora-coreos-tracker#272

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was chatting with @yuvalturg, and IIUC, the root directory itself is also affected to this. I.e. I think we also need:

diff --git a/src/app/rpmostree-compose-builtin-tree.c b/src/app/rpmostree-compose-builtin-tree.c
index 6bb48f39..89b70d3d 100644
--- a/src/app/rpmostree-compose-builtin-tree.c
+++ b/src/app/rpmostree-compose-builtin-tree.c
@@ -943,6 +943,9 @@ impl_install_tree (RpmOstreeTreeComposeContext *self,
     return FALSE;
   if (!glnx_ensure_dir (self->workdir_dfd, final_rootfs_name, 0755, error))
     return FALSE;
+  /* make sure umask isn't affecting us */
+  if (fchmodat (dfd, toplevel_dirs[i], 0755, 0) == -1)
+    return glnx_throw_errno_prefix (error, "fchmodat");
   { glnx_autofd int target_rootfs_dfd = -1;
     if (!glnx_opendirat (self->workdir_dfd, final_rootfs_name, TRUE,
                          &target_rootfs_dfd, error))

?

`umask` is one of those really evil Unix things...it's pretty
crazy actually there's still no threadsafe way to "`mkdir` ignoring umask".

This surfaced in someone using coreos-assembler with a working
directory of mode `0750` and having that surface in the target
rootfs.

Ref: coreos/fedora-coreos-tracker#272
@yuvalturg
Copy link

Right, the final ostree looks like this:

d00750 0 0 0 /
l00777 0 0 0 /bin -> usr/bin
l00777 0 0 0 /home -> var/home
l00777 0 0 0 /lib -> usr/lib
l00777 0 0 0 /lib64 -> usr/lib64
l00777 0 0 0 /media -> run/media
l00777 0 0 0 /mnt -> var/mnt
l00777 0 0 0 /opt -> var/opt
l00777 0 0 0 /ostree -> sysroot/ostree
l00777 0 0 0 /root -> var/roothome
l00777 0 0 0 /sbin -> usr/sbin
l00777 0 0 0 /srv -> var/srv
d00750 0 0 0 /boot
d00750 0 0 0 /dev
d00750 0 0 0 /proc
d00750 0 0 0 /run
d00750 0 0 0 /sys
d00750 0 0 0 /sysroot
d01777 0 0 0 /tmp
d00750 0 0 0 /usr
d00750 0 0 0 /var

Thanks for all the assistance and the quick response @jlebon @cgwalters ! :)

@cgwalters
Copy link
Member Author

I.e. I think we also need:

I did a different fix because there's similar code in the rojig path. I probably should have noted that I didn't try reproducing the failure, but I am doing so now.

@yuvalturg
Copy link

glnx_ensure_dir

s/dfd, toplevel_dirs[i]/self->workdir_dfd, final_rootfs_name/

@jlebon
Copy link
Member

jlebon commented Sep 11, 2019

@rh-atomic-bot r+ 5e6edd5

@rh-atomic-bot
Copy link

⌛ Testing commit 5e6edd5 with merge d06723d...

rh-atomic-bot pushed a commit that referenced this pull request Sep 11, 2019
`umask` is one of those really evil Unix things...it's pretty
crazy actually there's still no threadsafe way to "`mkdir` ignoring umask".

This surfaced in someone using coreos-assembler with a working
directory of mode `0750` and having that surface in the target
rootfs.

Ref: coreos/fedora-coreos-tracker#272

Closes: #1902
Approved by: jlebon
@rh-atomic-bot
Copy link

💔 Test failed - status-atomicjenkins

@jlebon
Copy link
Member

jlebon commented Sep 11, 2019

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 5e6edd5 with merge 13377d4...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 13377d4 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants