Permalink
Browse files

Don't try to create dirs if they already exists

  • Loading branch information...
1 parent 3c21459 commit 82867229abe9a85a0ca729fed8ec6a6133ccb6bc @spastorino spastorino committed Aug 11, 2010
Showing with 14 additions and 10 deletions.
  1. +2 −2 lib/bundler.rb
  2. +2 −2 lib/bundler/runtime.rb
  3. +1 −1 lib/bundler/settings.rb
  4. +9 −5 lib/bundler/source.rb
View
@@ -82,7 +82,7 @@ def bin_path
@bin_path ||= begin
path = settings[:bin] || "bin"
path = Pathname.new(path).expand_path(root)
- FileUtils.mkdir_p(path)
+ FileUtils.mkdir_p(path) unless File.exist?(path)
Pathname.new(path).expand_path
end
end
@@ -202,7 +202,7 @@ def mkdir_p(path)
sudo "mkdir -p '#{path}'"
else
FileUtils.mkdir_p(path)
- end
+ end unless File.exist?(path)
end
def sudo(str)
View
@@ -81,7 +81,7 @@ def dependencies_for(*groups)
alias gems specs
def cache
- FileUtils.mkdir_p(cache_path)
+ FileUtils.mkdir_p(cache_path) unless File.exist?(cache_path)
Bundler.ui.info "Updating .gem files in vendor/cache"
specs.each do |spec|
@@ -92,7 +92,7 @@ def cache
end
def prune_cache
- FileUtils.mkdir_p(cache_path)
+ FileUtils.mkdir_p(cache_path) unless File.exist?(cache_path)
resolve = @definition.resolve
cached = Dir["#{cache_path}/*.gem"]
View
@@ -99,7 +99,7 @@ def set_key(key, value, hash, file)
unless hash[key] == value
hash[key] = value
hash.delete(key) if value.nil?
- FileUtils.mkdir_p(file.dirname)
+ FileUtils.mkdir_p(file.dirname) unless File.exist?(file.dirname)
File.open(file, "w") { |f| f.puts hash.to_yaml }
end
value
View
@@ -242,7 +242,7 @@ def download_gem_from_uri(spec, uri)
download_path = Bundler.requires_sudo? ? Bundler.tmp : Gem.dir
gem_path = "#{Gem.dir}/cache/#{spec.full_name}.gem"
- FileUtils.mkdir_p("#{download_path}/cache")
+ FileUtils.mkdir_p("#{download_path}/cache") unless File.exist?("#{download_path}/cache")
Gem::RemoteFetcher.fetcher.download(spec, uri, download_path)
if Bundler.requires_sudo?
@@ -391,10 +391,14 @@ def initialize(spec, options = {})
def generate_bin
return if spec.executables.nil? || spec.executables.empty?
- FileUtils.mkdir_p("#{Bundler.tmp}/bin") if Bundler.requires_sudo?
+ if Bundler.requires_sudo?
+ FileUtils.mkdir_p("#{Bundler.tmp}/bin") unless File.exist?("#{Bundler.tmp}/bin")
+ end
+
super
+
if Bundler.requires_sudo?
- Bundler.mkdir_p "#{Gem.dir}/bin"
+ Bundler.mkdir_p("#{Gem.dir}/bin") unless File.exist?("#{Gem.dir}/bin")
spec.executables.each do |exe|
Bundler.sudo "cp -R #{Bundler.tmp}/bin/#{exe} #{Gem.dir}/bin/"
end
@@ -614,14 +618,14 @@ def cache
in_cache { git %|fetch --force --quiet "#{uri}" refs/heads/*:refs/heads/*| }
else
Bundler.ui.info "Fetching #{uri}"
- FileUtils.mkdir_p(cache_path.dirname)
+ FileUtils.mkdir_p(cache_path.dirname) unless File.exist?(cache_path.dirname)
git %|clone "#{uri}" "#{cache_path}" --bare --no-hardlinks|
end
end
def checkout
unless File.exist?(path.join(".git"))
- FileUtils.mkdir_p(path.dirname)
+ FileUtils.mkdir_p(path.dirname) unless File.exist?(path.dirname)
git %|clone --no-checkout "#{cache_path}" "#{path}"|
end
Dir.chdir(path) do

9 comments on commit 8286722

Now you have a race condition at each of these sites because the path can be deleted in between the existence test and the mkdir_p call.

Seeing as mkdir_p on an already existing directory is just a harmless no-op, and none of these sites are in performance-critical hotspots, isn't the way it was before this patch actually better?

yep, mkdir -p or mkdir_p is harmless on an existing directory. Hadn't thought about the race, but even without that, this commit just makes things unnecessarily noisy.

Owner

indirect replied Aug 13, 2010

No, the way it was before isn't better. This fixes a bug in the sudo logic. Also, a race condition? With what? This isn't running in a thread. If you mean there's a "race condition" with the entire operating system... uhh... every single program, ever, that does anything with files at all has that exact same "race condition". Seems like the only way to solve that is to never touch the filesystem from our programs, and that strikes me as unlikely.

That said mkdir_p really is a noop, I talked to Santiago, and he made a new patch. Thanks for the discussion.

ok, well +1 for better commit comments. I can understand if you had a problem with sudo logic and wanted to avoid the mkdir_p call, but without that context, this didn't make any sense.

The race condition is the one I referred to in my original comment: "the path can be deleted between the existence test and the mkdir_p call" (deleted by another process, another user, whatever).

In general any code which follows a "check if I can do something, then actually do it" pattern will be vulnerable to this kind of race. Good "defensive programming" style avoids this by instead adopting a "try to do something, and be ready to handle failure if I can't" pattern instead.

Owner

indirect replied Aug 13, 2010

My point exactly. If some other process were to delete the directory after the call to File.exist?, it would likewise be deleting it after the call to FileUtils.mkdir_p if the call to File.exist? was removed. So the so-called "race condition" that you are referring to would have to be handled by the code that uses the directory either way, and it has nothing to do with checking for the existence of anything.

It's not a "so-called" race condition at all. Any code that involves the pattern of "check if I can do something, and if I can then proceed to do it" is going to suffer a race condition, unless you can specifically guarantee that in the time that elapses between the check and the subsequent action, that the thing you checked hasn't changed behind your back.

This is a standard pattern which occurs absolutely all over the place in computer science. Look at the mkdtemp (3) manpage for a discussion of exactly this kind of race condition. Standard practice to avoid this is, as I said, to not use the "check then do" pattern but instead use the "try to do it, and be ready to handle failure if it didn't work" pattern.

Just to be clear, I am not arguing that this "race condition" is a security flaw or anything like that. I'm just saying that because it is a race, using this "check then do" pattern provides a false sense of security about whether or not we actually need to create the directory. As I said in my initial comment, seeing as mkdir_p on an existing directory is a harmless no-op, the original code was better because it didn't do any harm, and didn't provide a false sense of security either.

As far as the commit message goes, the actual motivation behind the commit was to fix a bug in the sudo logic, but wasn't mentioned in the commit message. And seeing as this bug has no accompanying specs, it's not evident from those either.

The commit which Santiago has since pushed has the same race condition, anyway. If your goal is that you want to avoid asking the user for their sudo password if the directory already exists, then the correct way to avoid the race and apply the "try and be ready to handle failure" pattern instead of the "check the do" pattern would probably be to: try to create the directory using mkdir_p without sudo, catch the Errno::ENOACCES exception if it fails, and then retry using sudo in that case.

Why? Because even if a particular case of the "check then do" pattern is harmless and doesn't have any security consequences, you don't want to get into the habit of using the pattern, as you never know when you'll get bitten by a case where it does have consequences. It's best to just follow the "try then handle failure" pattern as a general rule everywhere where a possible race exists.

Member

spastorino replied Aug 14, 2010

wincent agree with you, feel free to patch it. BTW i think you'd like to patch rubygems too http://github.com/ruby/ruby/blob/trunk/lib/rubygems/installer.rb#L294 the same pattern was followed all over the place there. Anyways i will never be against the best solutions so please go ahead.

Yeah definitely seems like most of the unless File.exist?(cache_path)s are unnecessary.

Please sign in to comment.