From bc03f21758b8825a0da22c4d1ab705975eb935b3 Mon Sep 17 00:00:00 2001 From: sophia Date: Thu, 12 Jan 2023 15:25:36 -0800 Subject: [PATCH 1/6] Add a file mutex when downloading box files. Box's are global to Vagrant. Multiple Vagrant process can all access the box directory for both downloading and extracting boxes. A file mutex will ensure that mulitple Vagrant process will not trample eachother if they are trying to download the same box. --- lib/vagrant/action/builtin/box_add.rb | 13 +++++++++++++ lib/vagrant/errors.rb | 4 ++++ templates/locales/en.yml | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/lib/vagrant/action/builtin/box_add.rb b/lib/vagrant/action/builtin/box_add.rb index c9307beea..ec8c04761 100644 --- a/lib/vagrant/action/builtin/box_add.rb +++ b/lib/vagrant/action/builtin/box_add.rb @@ -22,6 +22,10 @@ module Vagrant # file already exists. RESUME_DELAY = 24 * 60 * 60 + # When a box file is being downloaded, identify the download is in progress + # by Vagrant with a mutex file named . + MUTEX_SUFFIX = ".lock" + def initialize(app, env) @app = app @logger = Log4r::Logger.new("vagrant::action::builtin::box_add") @@ -478,13 +482,22 @@ module Vagrant end end + mutex_path = d.destination + MUTEX_SUFFIX + if File.file?(mutex_path) + raise Errors::DownloadAlreadyInProgress, + dest_path: d.destination + end + begin + File.write(mutex_path, "") d.download! rescue Errors::DownloaderInterrupted # The downloader was interrupted, so just return, because that # means we were interrupted as well. @download_interrupted = true env[:ui].info(I18n.t("vagrant.actions.box.download.interrupted")) + ensure + File.delete(mutex_path) end Pathname.new(d.destination) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 664da86e5..186de6865 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -376,6 +376,10 @@ module Vagrant error_key(:dotfile_upgrade_json_error) end + class DownloadAlreadyInProgress < VagrantError + error_key(:download_already_in_progress_error) + end + class DownloaderError < VagrantError error_key(:downloader_error) end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 9e6d7ca16..a20c6ce79 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -895,6 +895,12 @@ en: support. State file path: %{state_file} + download_already_in_progress_error: |- + Download to global Vagrant location already in progress. This + may be caused by other Vagrant processes attempting to download + a file to the same location. + + Download path: %{dest_path} downloader_error: |- An error occurred while downloading the remote file. The error message, if any, is reproduced below. Please fix this error and try From 83efa09dfafe3ed26ea4c2321bd1af9f0ccd277b Mon Sep 17 00:00:00 2001 From: sophia Date: Thu, 12 Jan 2023 16:30:05 -0800 Subject: [PATCH 2/6] Test box download mutex --- .../vagrant/action/builtin/box_add_test.rb | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/unit/vagrant/action/builtin/box_add_test.rb b/test/unit/vagrant/action/builtin/box_add_test.rb index 67652d501..397ff1259 100644 --- a/test/unit/vagrant/action/builtin/box_add_test.rb +++ b/test/unit/vagrant/action/builtin/box_add_test.rb @@ -84,7 +84,47 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows, :bsdtar do allow(box_collection).to receive(:find).and_return(nil) end + context "the download location is locked" do + let(:box_path) { iso_env.box2_file(:virtualbox) } + + before do + mutex_path = env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s + File.write(mutex_path, "") + end + + it "raises a download error" do + env[:box_name] = "foo" + env[:box_url] = box_path.to_s + + expect { subject.call(env) }. + to raise_error(Vagrant::Errors::DownloadAlreadyInProgress) + end + end + context "with box file directly" do + it "creates and cleans up a lock file" do + box_path = iso_env.box2_file(:virtualbox) + + env[:box_name] = "foo" + env[:box_url] = box_path.to_s + + + expect(box_collection).to receive(:add).with(any_args) { |path, name, version, opts| + expect(checksum(path)).to eq(checksum(box_path)) + expect(name).to eq("foo") + expect(version).to eq("0") + expect(opts[:metadata_url]).to be_nil + true + }.and_return(box) + + expect(app).to receive(:call).with(env) + + mutex_path = env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s + expect(File).to receive(:write).with(mutex_path, "") + expect(File).to receive(:delete).with(mutex_path) + subject.call(env) + end + it "adds it" do box_path = iso_env.box2_file(:virtualbox) From 817fbdd2d132ab6636234387d67ae56ae1eee7d3 Mon Sep 17 00:00:00 2001 From: sophia Date: Thu, 12 Jan 2023 16:34:45 -0800 Subject: [PATCH 3/6] Add information about lock file path --- lib/vagrant/action/builtin/box_add.rb | 3 ++- templates/locales/en.yml | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/vagrant/action/builtin/box_add.rb b/lib/vagrant/action/builtin/box_add.rb index ec8c04761..80d5b10ce 100644 --- a/lib/vagrant/action/builtin/box_add.rb +++ b/lib/vagrant/action/builtin/box_add.rb @@ -485,7 +485,8 @@ module Vagrant mutex_path = d.destination + MUTEX_SUFFIX if File.file?(mutex_path) raise Errors::DownloadAlreadyInProgress, - dest_path: d.destination + dest_path: d.destination, + lock_file_path: mutex_path end begin diff --git a/templates/locales/en.yml b/templates/locales/en.yml index a20c6ce79..095f83384 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -901,6 +901,7 @@ en: a file to the same location. Download path: %{dest_path} + Lock file path: %{lock_file_path} downloader_error: |- An error occurred while downloading the remote file. The error message, if any, is reproduced below. Please fix this error and try From b91a5d5576f9df54ab64365620ef412b60e5aba2 Mon Sep 17 00:00:00 2001 From: sophia Date: Fri, 13 Jan 2023 11:46:00 -0800 Subject: [PATCH 4/6] Add file mutex util module --- lib/vagrant/action/builtin/box_add.rb | 32 ++++++++++++--------------- lib/vagrant/errors.rb | 4 ++++ lib/vagrant/util.rb | 1 + lib/vagrant/util/file_mutex.rb | 29 ++++++++++++++++++++++++ templates/locales/en.yml | 5 +++++ 5 files changed, 53 insertions(+), 18 deletions(-) create mode 100644 lib/vagrant/util/file_mutex.rb diff --git a/lib/vagrant/action/builtin/box_add.rb b/lib/vagrant/action/builtin/box_add.rb index 80d5b10ce..3b5624c4f 100644 --- a/lib/vagrant/action/builtin/box_add.rb +++ b/lib/vagrant/action/builtin/box_add.rb @@ -6,6 +6,7 @@ require "uri" require "vagrant/box_metadata" require "vagrant/util/downloader" require "vagrant/util/file_checksum" +require "vagrant/util/file_mutex" require "vagrant/util/platform" module Vagrant @@ -22,10 +23,6 @@ module Vagrant # file already exists. RESUME_DELAY = 24 * 60 * 60 - # When a box file is being downloaded, identify the download is in progress - # by Vagrant with a mutex file named . - MUTEX_SUFFIX = ".lock" - def initialize(app, env) @app = app @logger = Log4r::Logger.new("vagrant::action::builtin::box_add") @@ -482,25 +479,24 @@ module Vagrant end end - mutex_path = d.destination + MUTEX_SUFFIX - if File.file?(mutex_path) + begin + mutex_path = d.destination + ".lock" + Util::FileMutex.new(mutex_path).with_lock do + begin + d.download! + rescue Errors::DownloaderInterrupted + # The downloader was interrupted, so just return, because that + # means we were interrupted as well. + @download_interrupted = true + env[:ui].info(I18n.t("vagrant.actions.box.download.interrupted")) + end + end + rescue Errors::VagrantLocked raise Errors::DownloadAlreadyInProgress, dest_path: d.destination, lock_file_path: mutex_path end - begin - File.write(mutex_path, "") - d.download! - rescue Errors::DownloaderInterrupted - # The downloader was interrupted, so just return, because that - # means we were interrupted as well. - @download_interrupted = true - env[:ui].info(I18n.t("vagrant.actions.box.download.interrupted")) - ensure - File.delete(mutex_path) - end - Pathname.new(d.destination) end diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 186de6865..8e6bb8989 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -924,6 +924,10 @@ module Vagrant error_key(:uploader_interrupted) end + class VagrantLocked < VagrantError + error_key(:vagrant_locked) + end + class VagrantInterrupt < VagrantError error_key(:interrupted) end diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index 4b3e0ff09..8e3cbd2af 100644 --- a/lib/vagrant/util.rb +++ b/lib/vagrant/util.rb @@ -20,6 +20,7 @@ module Vagrant autoload :Experimental, 'vagrant/util/experimental' autoload :FileChecksum, 'vagrant/util/file_checksum' autoload :FileMode, 'vagrant/util/file_mode' + autoload :FileMutex, 'vagrant/util/file_mutex' autoload :GuestHosts, 'vagrant/util/guest_hosts' autoload :GuestInspection, 'vagrant/util/guest_inspection' autoload :HashWithIndifferentAccess, 'vagrant/util/hash_with_indifferent_access' diff --git a/lib/vagrant/util/file_mutex.rb b/lib/vagrant/util/file_mutex.rb new file mode 100644 index 000000000..c005a23a6 --- /dev/null +++ b/lib/vagrant/util/file_mutex.rb @@ -0,0 +1,29 @@ +module Vagrant + module Util + class FileMutex + def initialize(mutex_path) + @mutex_path = mutex_path + end + + def with_lock(&block) + lock + block.call + ensure + unlock + end + + def lock + if File.file?(@mutex_path) + raise Errors::VagrantLocked, + lock_file_path: @mutex_path + end + + File.write(@mutex_path, "") + end + + def unlock + File.delete(@mutex_path) + end + end + end +end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 095f83384..4a18ff7f6 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1675,6 +1675,11 @@ en: uploader_interrupted: |- The upload was interrupted by an external signal. It did not complete. + vagrant_locked: |- + The requested Vagrant action is locked. This may be caused + by other Vagrant processes attempting to do a similar action. + + Lock file path: %{lock_file_path} vagrantfile_exists: |- `Vagrantfile` already exists in this directory. Remove it before running `vagrant init`. From f153996b2d7480feecfc2c98136b69dbc5f6978d Mon Sep 17 00:00:00 2001 From: sophia Date: Fri, 13 Jan 2023 13:27:48 -0800 Subject: [PATCH 5/6] Add tests for mutex util module --- lib/vagrant/util/file_mutex.rb | 2 +- .../vagrant/action/builtin/box_add_test.rb | 23 --------- test/unit/vagrant/util/file_mutex_test.rb | 49 +++++++++++++++++++ 3 files changed, 50 insertions(+), 24 deletions(-) create mode 100644 test/unit/vagrant/util/file_mutex_test.rb diff --git a/lib/vagrant/util/file_mutex.rb b/lib/vagrant/util/file_mutex.rb index c005a23a6..2489d1a47 100644 --- a/lib/vagrant/util/file_mutex.rb +++ b/lib/vagrant/util/file_mutex.rb @@ -22,7 +22,7 @@ module Vagrant end def unlock - File.delete(@mutex_path) + File.delete(@mutex_path) if File.file?(@mutex_path) end end end diff --git a/test/unit/vagrant/action/builtin/box_add_test.rb b/test/unit/vagrant/action/builtin/box_add_test.rb index 397ff1259..2b53a7f34 100644 --- a/test/unit/vagrant/action/builtin/box_add_test.rb +++ b/test/unit/vagrant/action/builtin/box_add_test.rb @@ -102,29 +102,6 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows, :bsdtar do end context "with box file directly" do - it "creates and cleans up a lock file" do - box_path = iso_env.box2_file(:virtualbox) - - env[:box_name] = "foo" - env[:box_url] = box_path.to_s - - - expect(box_collection).to receive(:add).with(any_args) { |path, name, version, opts| - expect(checksum(path)).to eq(checksum(box_path)) - expect(name).to eq("foo") - expect(version).to eq("0") - expect(opts[:metadata_url]).to be_nil - true - }.and_return(box) - - expect(app).to receive(:call).with(env) - - mutex_path = env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s - expect(File).to receive(:write).with(mutex_path, "") - expect(File).to receive(:delete).with(mutex_path) - subject.call(env) - end - it "adds it" do box_path = iso_env.box2_file(:virtualbox) diff --git a/test/unit/vagrant/util/file_mutex_test.rb b/test/unit/vagrant/util/file_mutex_test.rb new file mode 100644 index 000000000..0a709f770 --- /dev/null +++ b/test/unit/vagrant/util/file_mutex_test.rb @@ -0,0 +1,49 @@ +require File.expand_path("../../../base", __FILE__) +require 'vagrant/util/file_mutex' + +describe Vagrant::Util::FileMutex do + include_context "unit" + + let(:temp_dir) { Dir.mktmpdir("vagrant-test-util-mutex_test") } + + after do + FileUtils.rm_rf(temp_dir) + end + + it "should create a lock file" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + instance.lock + expect(File).to exist(mutex_path) + end + + it "should create and delete lock file" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + instance.lock + instance.unlock + expect(File).to_not exist(mutex_path) + end + + it "should not raise an error if the lock file does not exist" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + instance.unlock + expect(File).to_not exist(mutex_path) + end + + it "should run a function with a lock" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + instance.with_lock { true } + expect(File).to_not exist(mutex_path) + end + + it "should fail running a function when locked" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + instance.lock + expect {instance.with_lock { true }}. + to raise_error(Vagrant::Errors::VagrantLocked) + end +end From 4551b8b2ad84bd1293b06d6731414f585959d7e7 Mon Sep 17 00:00:00 2001 From: sophia Date: Thu, 2 Feb 2023 16:42:24 -0800 Subject: [PATCH 6/6] Use file locks to avoid file existance checking race conditions --- lib/vagrant/util/file_mutex.rb | 18 +++++----- .../vagrant/action/builtin/box_add_test.rb | 10 ++++-- test/unit/vagrant/util/file_mutex_test.rb | 34 ++++++++++++++++++- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/lib/vagrant/util/file_mutex.rb b/lib/vagrant/util/file_mutex.rb index 2489d1a47..0844726dd 100644 --- a/lib/vagrant/util/file_mutex.rb +++ b/lib/vagrant/util/file_mutex.rb @@ -7,18 +7,20 @@ module Vagrant def with_lock(&block) lock - block.call - ensure - unlock + begin + block.call + rescue => e + raise e + ensure + unlock + end end def lock - if File.file?(@mutex_path) - raise Errors::VagrantLocked, - lock_file_path: @mutex_path + f = File.open(@mutex_path, "w+") + if f.flock(File::LOCK_EX|File::LOCK_NB) === false + raise Errors::VagrantLocked, lock_file_path: @mutex_path end - - File.write(@mutex_path, "") end def unlock diff --git a/test/unit/vagrant/action/builtin/box_add_test.rb b/test/unit/vagrant/action/builtin/box_add_test.rb index 2b53a7f34..a28bcb16a 100644 --- a/test/unit/vagrant/action/builtin/box_add_test.rb +++ b/test/unit/vagrant/action/builtin/box_add_test.rb @@ -86,12 +86,16 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows, :bsdtar do context "the download location is locked" do let(:box_path) { iso_env.box2_file(:virtualbox) } + let(:mutex_path) { env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s } before do - mutex_path = env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s - File.write(mutex_path, "") + # Lock file + @f = File.open(mutex_path, "w+") + @f.flock(File::LOCK_EX|File::LOCK_NB) end - + + after { @f.close } + it "raises a download error" do env[:box_name] = "foo" env[:box_url] = box_path.to_s diff --git a/test/unit/vagrant/util/file_mutex_test.rb b/test/unit/vagrant/util/file_mutex_test.rb index 0a709f770..eacbd9ffd 100644 --- a/test/unit/vagrant/util/file_mutex_test.rb +++ b/test/unit/vagrant/util/file_mutex_test.rb @@ -21,6 +21,7 @@ describe Vagrant::Util::FileMutex do mutex_path = temp_dir + "test.lock" instance = described_class.new(mutex_path) instance.lock + expect(File).to exist(mutex_path) instance.unlock expect(File).to_not exist(mutex_path) end @@ -41,9 +42,40 @@ describe Vagrant::Util::FileMutex do it "should fail running a function when locked" do mutex_path = temp_dir + "test.lock" + # create a lock instance = described_class.new(mutex_path) instance.lock - expect {instance.with_lock { true }}. + # create a new lock that will run a function + instance2 = described_class.new(mutex_path) + # lock should persist for multiple runs + expect {instance2.with_lock { true }}. to raise_error(Vagrant::Errors::VagrantLocked) + expect {instance2.with_lock { true }}. + to raise_error(Vagrant::Errors::VagrantLocked) + # mutex should exist until its unlocked + expect(File).to exist(mutex_path) + instance.unlock + expect(File).to_not exist(mutex_path) + end + + it "should fail running a function within a locked" do + mutex_path = temp_dir + "test.lock" + # create a lock + instance = described_class.new(mutex_path) + # create a new lock that will run a function + instance2 = described_class.new(mutex_path) + expect { + instance.with_lock { instance2.with_lock{true} } + }.to raise_error(Vagrant::Errors::VagrantLocked) + expect(File).to_not exist(mutex_path) + end + + it "should delete the lock even when the function fails" do + mutex_path = temp_dir + "test.lock" + instance = described_class.new(mutex_path) + expect { + instance.with_lock { raise Vagrant::Errors::VagrantError.new } + }.to raise_error(Vagrant::Errors::VagrantError) + expect(File).to_not exist(mutex_path) end end