Navigation Menu

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

Cache permissions and umask issues after switch to mkstemp #255

Merged
merged 4 commits into from Apr 2, 2019
Merged

Cache permissions and umask issues after switch to mkstemp #255

merged 4 commits into from Apr 2, 2019

Conversation

kpumuk
Copy link
Contributor

@kpumuk kpumuk commented Apr 2, 2019

This PR resolves multiple permissions-related issues introduced in #240.

State before the change

  • When directory does not exist, it is being created with mkpath (respects umask, default permissions 0775), and then file opened with open (respects umask, default permissions are 0664).
  • When the second file is created in the same directory, it uses mkstemp, which always creates files with permissions 0600. This causes various issues when permissions system relies on user groups, as the group can no longer read or write cache files.

After this change

  • After directory first created, we use open to create first file.
  • The second file is created with mkstemp call, which sets permissions to 0600.
  • To fix permissions, we apply 0644 to the file after rename, and properly use umask value (current umask is determined in the Init_bootsnap)

I have tested this change with and without umask, and it seems to be working properly:

  • By default directories are created with 0775
  • Files are created with 0664
  • When umask 022 is specified, directories are 0755 and files are 0644

Previous behavior that was reverted by mkstemp

Related issues

Updates

I was wrong about the second open call, it makes sense now and PR is corrected. ;-) The pemissions issue is still there.

@kpumuk kpumuk requested a review from burke as a code owner April 2, 2019 18:57
@ghost ghost added the cla-needed label Apr 2, 2019
@ghost ghost removed the cla-needed label Apr 2, 2019
Copy link
Member

@burke burke left a comment

Choose a reason for hiding this comment

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

Oh, this looks great. Thank you!

@burke burke merged commit 55e048e into Shopify:master Apr 2, 2019
@kpumuk
Copy link
Contributor Author

kpumuk commented Apr 2, 2019

Thank you!

@markets
Copy link

markets commented Apr 5, 2019

Thanks a lot for working on this! 🙌

We're having the same issue deploying with Capistrano a newish Rails 5.2 app with Bootsnap 1.4.2, so we temporary disabled Bootsnap. Any plans to cut a new release?

@ansonhoyt
Copy link
Contributor

@markets 1.4.3 has been released. I'm re-enabling in production now too.

@markets
Copy link

markets commented Apr 9, 2019

Thanks @ansonhoyt will try asap!

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 this pull request may close these issues.

None yet

5 participants