This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
use /tmp for mktmpdir #5043
Merged
use /tmp for mktmpdir #5043
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
As we noticed in #4519, we need to use a temporary directory to hold compact index downloads so that multiple processes don't write to the same files at the same time and break everything. The fix for that was #4561, which added temporary directories to hold all files as they download, and then uses the (atomic) `FileUtils.cp` to move the completed downloads into place, so there is never a point where multiple processes are trying to write into the file at once. Unfortunately, using `Dir.mktmpdir` requires that the parent directory be _either_ world writable or sticky, but not both. Based on #4599, it looks like it's common for home directories to be both world writable and sticky. While that's a security problem by itself, it's not a big concern for Bundler and the compact index. So we want to let users continue to use Bundler, even with the compact index, without having to change the permissions on their home directories. This commit changes the `mktmpdir` call to create the temporary directory inside the default OS tempdir, which is typically `/tmp` or `/var/tmp` depending on distro. Since that directory is designed to hold other temporary directories, that change should (theoretically) reduce or eliminate the problem reported in #4599.
|
@indirect any chance for a test case on this? |
|
@segiddins yeah, working on it |
|
@segiddins test supplied, pls to review |
|
did you confirm the test fails without the fix? |
|
@segiddins yes, I wrote a failing test against master and then committed this test on the branch after ensuring it changed to green. |
|
@homu r+ |
|
📌 Commit 0954697 has been approved by |
homu
added a commit
that referenced
this pull request
Oct 4, 2016
use /tmp for mktmpdir As we noticed in #4519, we need to use a temporary directory to hold compact index downloads so that multiple processes don't write to the same files at the same time and break everything. The fix for that was #4561, which added temporary directories to hold all files as they download, and then uses the (atomic) `FileUtils.cp` to move the completed downloads into place, so there is never a point where multiple processes are trying to write into the file at once. Unfortunately, using `Dir.mktmpdir` requires that the parent directory be _either_ world writable or sticky, but not both. Based on #4599, it looks like it's common for home directories to be both world writable and sticky. While that's a security problem by itself, it's not a big concern for Bundler and the compact index. So we want to let users continue to use Bundler, even with the compact index, without having to change the permissions on their home directories. This commit changes the `mktmpdir` call to create the temporary directory inside the default OS tempdir, which is typically `/tmp` or `/var/tmp` depending on distro. Since that directory is designed to hold other temporary directories, that change should (theoretically) reduce or eliminate the problem reported in #4599.
|
💔 Test failed - status |
|
restarted the 1.8.7 build, didn't think to check if I actually broke 1.8 until after the log was gone >_> |
|
☀️ Test successful - status |
indirect
pushed a commit
that referenced
this pull request
Oct 11, 2016
use /tmp for mktmpdir As we noticed in #4519, we need to use a temporary directory to hold compact index downloads so that multiple processes don't write to the same files at the same time and break everything. The fix for that was #4561, which added temporary directories to hold all files as they download, and then uses the (atomic) `FileUtils.cp` to move the completed downloads into place, so there is never a point where multiple processes are trying to write into the file at once. Unfortunately, using `Dir.mktmpdir` requires that the parent directory be _either_ world writable or sticky, but not both. Based on #4599, it looks like it's common for home directories to be both world writable and sticky. While that's a security problem by itself, it's not a big concern for Bundler and the compact index. So we want to let users continue to use Bundler, even with the compact index, without having to change the permissions on their home directories. This commit changes the `mktmpdir` call to create the temporary directory inside the default OS tempdir, which is typically `/tmp` or `/var/tmp` depending on distro. Since that directory is designed to hold other temporary directories, that change should (theoretically) reduce or eliminate the problem reported in #4599.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
As we noticed in #4519, we need to use a temporary directory to hold
compact index downloads so that multiple processes don't write to the
same files at the same time and break everything.
The fix for that was #4561, which added temporary directories to hold
all files as they download, and then uses the (atomic)
FileUtils.cptomove the completed downloads into place, so there is never a point where
multiple processes are trying to write into the file at once.
Unfortunately, using
Dir.mktmpdirrequires that the parent directorybe either world writable or sticky, but not both. Based on #4599, it
looks like it's common for home directories to be both world writable
and sticky. While that's a security problem by itself, it's not a big
concern for Bundler and the compact index. So we want to let users
continue to use Bundler, even with the compact index, without having to
change the permissions on their home directories.
This commit changes the
mktmpdircall to create the temporarydirectory inside the default OS tempdir, which is typically
/tmpor/var/tmpdepending on distro. Since that directory is designed to holdother temporary directories, that change should (theoretically) reduce
or eliminate the problem reported in #4599.