This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Safely store concurrent compact index downloads #4561
Merged
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
|
Could you try and add some test coverage for this? |
| return if response.is_a?(Net::HTTPNotModified) | ||
| # download new file if retrying | ||
| if retrying.nil? && local_path.file? | ||
| FileUtils.cp local_path, local_temp_path |
There was a problem hiding this comment.
It ensures the view of local_path is consistent through the whole process - if the etag and .size were taken of the original local_path, then it might change during execution with another parallel process.
It also ensures that when we append the response to the file that it's for the same matching content. If it had changed in the meantime then it'd be corrupt.
|
These changes generally look good to me |
domcleal
force-pushed
the
4519-concurrent-ci-updater
branch
from
0c933f5 to
a0900c8
Compare
I've added a test around bundle install that emulates a concurrent modification of the cached file during the HTTP request, then verifies that a corrupt file wasn't written to disk. |
|
Awesome, thanks! |
domcleal
force-pushed
the
4519-concurrent-ci-updater
branch
from
a0900c8 to
d13ba6a
Compare
When bundler is run concurrently using the same bundle dir in $HOME, the versions file can be updated from two processes at once. The download has been changed to a temporary file, which is securely moved into place over the original. If retrying the update operation, the original file is no longer immediately deleted and instead a full download is performed, later overwriting the original file if successful. If two processes are updating in parallel, this should ensure the original file isn't corrupted and that both processes succeed. - Fixes rubygems#4519
domcleal
force-pushed
the
4519-concurrent-ci-updater
branch
from
d13ba6a to
97bf593
Compare
|
@indirect r? |
|
This looks great, thanks a lot! |
|
@homu r+ |
|
📌 Commit 97bf593 has been approved by |
homu
added a commit
that referenced
this pull request
May 15, 2016
Safely store concurrent compact index downloads When bundler is run concurrently using the same bundle dir in $HOME, the versions file can be updated from two processes at once. The download has been changed to a temporary file, which is securely moved into place over the original. If retrying the update operation, the original file is no longer immediately deleted and instead a full download is performed, later overwriting the original file if successful. If two processes are updating in parallel, this should ensure the original file isn't corrupted and that both processes succeed. - Fixes #4519 --- This would be useful on 1.12.x if possible, since the new caching behaviour with a shared home directory is causing the errors described in #4519.
|
☀️ Test successful - status |
segiddins
pushed a commit
that referenced
this pull request
May 16, 2016
Safely store concurrent compact index downloads When bundler is run concurrently using the same bundle dir in $HOME, the versions file can be updated from two processes at once. The download has been changed to a temporary file, which is securely moved into place over the original. If retrying the update operation, the original file is no longer immediately deleted and instead a full download is performed, later overwriting the original file if successful. If two processes are updating in parallel, this should ensure the original file isn't corrupted and that both processes succeed. - Fixes #4519 --- This would be useful on 1.12.x if possible, since the new caching behaviour with a shared home directory is causing the errors described in #4519.
indirect
added a commit
that referenced
this pull request
Oct 3, 2016
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.
Merged
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.
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.
When bundler is run concurrently using the same bundle dir in $HOME,
the versions file can be updated from two processes at once. The
download has been changed to a temporary file, which is securely moved
into place over the original.
If retrying the update operation, the original file is no longer
immediately deleted and instead a full download is performed, later
overwriting the original file if successful.
If two processes are updating in parallel, this should ensure the
original file isn't corrupted and that both processes succeed.
This would be useful on 1.12.x if possible, since the new caching behaviour with a shared home directory is causing the errors described in #4519.