From aba2e7dc0909261e7f5a9cd6f39f515de857e2b9 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 3 Nov 2014 20:27:59 -0500 Subject: [PATCH] Do not leak tempdirs Be nice and clean up after yo'self --- lib/berkshelf.rb | 15 ----- lib/berkshelf/berksfile.rb | 67 +++++++++++----------- lib/berkshelf/community_rest.rb | 6 +- lib/berkshelf/downloader.rb | 13 ++++- lib/berkshelf/installer.rb | 5 +- spec/unit/berkshelf/community_rest_spec.rb | 6 +- 6 files changed, 57 insertions(+), 55 deletions(-) diff --git a/lib/berkshelf.rb b/lib/berkshelf.rb index 2a54d1ee3..f0f4cfcb7 100644 --- a/lib/berkshelf.rb +++ b/lib/berkshelf.rb @@ -8,7 +8,6 @@ require 'semverse' require 'solve' require 'thor' -require 'tmpdir' require 'uri' require 'celluloid' @@ -105,20 +104,6 @@ def initialize_filesystem end end - # @return [String] - def tmp_dir - File.join(berkshelf_path, 'tmp') - end - - # Creates a temporary directory within the Berkshelf path - # - # @return [String] - # path to the created temporary directory - def mktmpdir - FileUtils.mkdir_p(tmp_dir) - Dir.mktmpdir(nil, tmp_dir) - end - # @return [Berkshelf::CookbookStore] def cookbook_store CookbookStore.instance diff --git a/lib/berkshelf/berksfile.rb b/lib/berkshelf/berksfile.rb index e231575c3..8f8aca5ac 100644 --- a/lib/berkshelf/berksfile.rb +++ b/lib/berkshelf/berksfile.rb @@ -574,48 +574,49 @@ def package(path) # @return [String, nil] # the expanded path cookbooks were vendored to or nil if nothing was vendored def vendor(destination) - scratch = Berkshelf.mktmpdir - chefignore = nil - cached_cookbooks = install + Dir.mktmpdir do |scratch| + chefignore = nil + cached_cookbooks = install - return nil if cached_cookbooks.empty? + return nil if cached_cookbooks.empty? - cached_cookbooks.each do |cookbook| - Berkshelf.formatter.vendor(cookbook, destination) - cookbook_destination = File.join(scratch, cookbook.cookbook_name) - FileUtils.mkdir_p(cookbook_destination) + cached_cookbooks.each do |cookbook| + Berkshelf.formatter.vendor(cookbook, destination) + cookbook_destination = File.join(scratch, cookbook.cookbook_name) + FileUtils.mkdir_p(cookbook_destination) - # Dir.glob does not support backslash as a File separator - src = cookbook.path.to_s.gsub('\\', '/') - files = FileSyncer.glob(File.join(src, '*')) + # Dir.glob does not support backslash as a File separator + src = cookbook.path.to_s.gsub('\\', '/') + files = FileSyncer.glob(File.join(src, '*')) - chefignore = Ridley::Chef::Chefignore.new(cookbook.path.to_s) rescue nil - chefignore.apply!(files) if chefignore + chefignore = Ridley::Chef::Chefignore.new(cookbook.path.to_s) rescue nil + chefignore.apply!(files) if chefignore - unless cookbook.compiled_metadata? - cookbook.compile_metadata(cookbook_destination) + unless cookbook.compiled_metadata? + cookbook.compile_metadata(cookbook_destination) + end + + FileUtils.cp_r(files, cookbook_destination) end - FileUtils.cp_r(files, cookbook_destination) + # Don't vendor the raw metadata (metadata.rb). The raw metadata is + # unecessary for the client, and this is required until compiled metadata + # (metadata.json) takes precedence over raw metadata in the Chef-Client. + # + # We can change back to including the raw metadata in the future after + # this has been fixed or just remove these comments. There is no + # circumstance that I can currently think of where raw metadata should + # ever be read by the client. + # + # - Jamie + # + # See the following tickets for more information: + # + # * https://tickets.opscode.com/browse/CHEF-4811 + # * https://tickets.opscode.com/browse/CHEF-4810 + FileSyncer.sync(scratch, destination, exclude: ["**/*/metadata.rb"]) end - # Don't vendor the raw metadata (metadata.rb). The raw metadata is - # unecessary for the client, and this is required until compiled metadata - # (metadata.json) takes precedence over raw metadata in the Chef-Client. - # - # We can change back to including the raw metadata in the future after - # this has been fixed or just remove these comments. There is no - # circumstance that I can currently think of where raw metadata should - # ever be read by the client. - # - # - Jamie - # - # See the following tickets for more information: - # - # * https://tickets.opscode.com/browse/CHEF-4811 - # * https://tickets.opscode.com/browse/CHEF-4810 - FileSyncer.sync(scratch, destination, exclude: ["**/*/metadata.rb"]) - destination end diff --git a/lib/berkshelf/community_rest.rb b/lib/berkshelf/community_rest.rb index 769e4b87d..d64facfc6 100644 --- a/lib/berkshelf/community_rest.rb +++ b/lib/berkshelf/community_rest.rb @@ -10,7 +10,7 @@ class << self # file path to extract the contents of the target to # # @return [String] - def unpack(target, destination = Dir.mktmpdir) + def unpack(target, destination) if is_gzip_file(target) Archive::Tar::Minitar.unpack(Zlib::GzipReader.new(File.open(target, 'rb')), destination) elsif is_tar_file(target) @@ -18,6 +18,7 @@ def unpack(target, destination = Dir.mktmpdir) else raise Berkshelf::UnknownCompressionType.new(target) end + FileUtils.rm_rf Dir.glob("#{destination}/**/PaxHeader") destination end @@ -100,7 +101,8 @@ def initialize(uri = V1_API, options = {}) # cookbook filepath, or nil if archive does not contain a cookbook def download(name, version) archive = stream(find(name, version)[:file]) - extracted = self.class.unpack(archive.path) + scratch = Dir.mktmpdir + extracted = self.class.unpack(archive.path, scratch) if File.cookbook?(extracted) extracted diff --git a/lib/berkshelf/downloader.rb b/lib/berkshelf/downloader.rb index 39ca7b426..80f703dad 100644 --- a/lib/berkshelf/downloader.rb +++ b/lib/berkshelf/downloader.rb @@ -15,7 +15,10 @@ def initialize(berksfile) @berksfile = berksfile end - # Download the given Berkshelf::Dependency. + # Download the given Berkshelf::Dependency. If the optional block is given, + # the temporary path to the cookbook is yielded and automatically deleted + # when the block returns. If no block is given, it is the responsibility of + # the caller to remove the tmpdir. # # @param [String] name # @param [String] version @@ -25,12 +28,18 @@ def initialize(berksfile) # @raise [CookbookNotFound] # # @return [String] - def download(*args) + def download(*args, &block) options = args.last.is_a?(Hash) ? args.pop : Hash.new dependency, version = args sources.each do |source| if result = try_download(source, dependency, version) + if block_given? + value = yield result + FileUtils.rm_rf(result) + return value + end + return result end end diff --git a/lib/berkshelf/installer.rb b/lib/berkshelf/installer.rb index c4e0665b2..8b911187a 100644 --- a/lib/berkshelf/installer.rb +++ b/lib/berkshelf/installer.rb @@ -102,8 +102,9 @@ def install(dependency) Berkshelf.formatter.install(source, cookbook) - stash = downloader.download(name, version) - CookbookStore.import(name, version, stash) + downloader.download(name, version) do |stash| + CookbookStore.import(name, version, stash) + end end end end diff --git a/spec/unit/berkshelf/community_rest_spec.rb b/spec/unit/berkshelf/community_rest_spec.rb index e329843ae..5e218534d 100644 --- a/spec/unit/berkshelf/community_rest_spec.rb +++ b/spec/unit/berkshelf/community_rest_spec.rb @@ -78,6 +78,7 @@ before do allow(subject).to receive(:stream).with(any_args()).and_return(archive) allow(Berkshelf::CommunityREST).to receive(:unpack) + .and_return('/some/path') end it 'unpacks the archive' do @@ -87,7 +88,10 @@ headers: { 'Content-Type' => 'application/json' }, ) - expect(Berkshelf::CommunityREST).to receive(:unpack).with('/foo/bar').once.and_return('/foo/nginx') + expect(Berkshelf::CommunityREST).to receive(:unpack) + .with('/foo/bar', String) + .and_return('/foo/nginx') + .once expect(archive).to receive(:unlink).once subject.download('bacon', '1.0.0')