From 23e9e92ebe82d055c2e225761b6e74a4fc80906a Mon Sep 17 00:00:00 2001 From: Kyle Butt Date: Tue, 17 Oct 2023 13:15:54 -0600 Subject: [PATCH 1/4] Add an option for returning a device similar to mkstemp If the temporary directory is world writable, in order to securely open a temporary file, you need to create it and open it at the same time and have exclusive access to it. It remains secure as long as you hold onto the file descriptor returned by open. Requesting a device returns the io device and the path, rather than just the path. Note that the io device is owned by the pid that called open(), and will be closed when that process exits. There is no way to hand off the device to another pid. If a temporary device needs to be long lived, it needs to be opened by a process that is at least as long lived. --- README.md | 2 +- guides/usage.livemd | 2 +- lib/briefly.ex | 6 +++- lib/briefly/config.ex | 2 ++ lib/briefly/entry.ex | 39 ++++++++++++++++++++---- test/lib/briefly_test.exs | 63 ++++++++++++++++++++++++++++++++++++++- 6 files changed, 104 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 0f5cf4c..e2e031d 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ Briefly can also create a temporary directory: ```elixir -{:ok, dir} = Briefly.create(directory: true) +{:ok, dir} = Briefly.create(type: :directory) File.write!(Path.join(dir, "test.txt"), "Some Text") # When this process exits, the directory and file are removed ``` diff --git a/guides/usage.livemd b/guides/usage.livemd index 073cfb2..3e30b2d 100644 --- a/guides/usage.livemd +++ b/guides/usage.livemd @@ -28,7 +28,7 @@ When this process exits, the file at `path` is removed. Briefly can also create a temporary directory: ```elixir -{:ok, dir} = Briefly.create(directory: true) +{:ok, dir} = Briefly.create(type: :directory) ``` You can use [`File.stat/1`](https://hexdocs.pm/elixir/File.html#stat/1) to check the type: diff --git a/lib/briefly.ex b/lib/briefly.ex index 14bfaea..5196898 100644 --- a/lib/briefly.ex +++ b/lib/briefly.ex @@ -16,13 +16,17 @@ defmodule Briefly do @type create_opts :: [ {:prefix, binary}, {:extname, binary}, + {:type, :path | :directory | :device}, {:directory, boolean} ] @doc """ Requests a temporary file to be created with the given options. """ - @spec create(create_opts) :: {:ok, binary} | {:error, Exception.t()} + @spec create(create_opts) :: + {:ok, binary} + | {:ok, binary, pid} + | {:error, Exception.t()} def create(opts \\ []) do opts |> Enum.into(%{}) diff --git a/lib/briefly/config.ex b/lib/briefly/config.ex index 7b7d0d2..81d61d0 100644 --- a/lib/briefly/config.ex +++ b/lib/briefly/config.ex @@ -3,6 +3,8 @@ defmodule Briefly.Config do def directory, do: get(:directory) + def directory_mode, do: get(:directory_mode) + def default_prefix, do: get(:default_prefix) def default_extname, do: get(:default_extname) diff --git a/lib/briefly/entry.ex b/lib/briefly/entry.ex index bd124d0..e9f0146 100644 --- a/lib/briefly/entry.ex +++ b/lib/briefly/entry.ex @@ -21,9 +21,14 @@ defmodule Briefly.Entry do def create(%{monitor_pid: pid} = options) do IO.warn("the :monitor_pid option is deprecated, please use Briefly.give_away/3 instead.") - with {:ok, path} <- create(Map.delete(options, :monitor_pid)) do - :ok = give_away(path, pid, self()) - {:ok, path} + case create(Map.delete(options, :monitor_pid)) do + {:ok, path} -> + :ok = give_away(path, pid, self()) + {:ok, path} + + {:ok, path, io_pid} -> + :ok = give_away(path, pid, self()) + {:ok, path, io_pid} end end @@ -36,13 +41,13 @@ defmodule Briefly.Entry do @doc false def cleanup(pid) do case :ets.lookup(@dir_table, pid) do - [{pid, _tmp}] -> + [{^pid, _tmp}] -> :ets.delete(@dir_table, pid) entries = :ets.lookup(@path_table, pid) Enum.each(entries, &delete_path/1) - for {_path, path} <- entries, do: path + for {_pid, path} <- entries, do: path [] -> [] @@ -161,7 +166,7 @@ defmodule Briefly.Entry do end end - defp open(%{directory: true} = options, tmp, attempts, _) when attempts < @max_attempts do + defp open(%{type: :directory} = options, tmp, attempts, _) when attempts < @max_attempts do path = path(options, tmp) case File.mkdir_p(path) do @@ -178,6 +183,28 @@ defmodule Briefly.Entry do end end + defp open(%{type: :device} = options, tmp, attempts, _) when attempts < @max_attempts do + path = path(options, tmp) + + case File.open(path, [:read, :write, :exclusive]) do + {:ok, device_pid} -> + :ets.insert(@path_table, {self(), path}) + {:ok, path, device_pid} + + {:error, reason} when reason in [:eexist, :eacces] -> + last_error = %Briefly.WriteError{code: reason, entry_type: :file, tmp_dir: tmp} + open(options, tmp, attempts + 1, last_error) + + {:error, code} -> + {:error, %Briefly.WriteError{code: code, entry_type: :file, tmp_dir: tmp}} + end + end + + defp open(%{directory: true} = options, tmp, attempts, last_error) do + new_opts = Map.put(options, :type, :directory) + open(new_opts, tmp, attempts, last_error) + end + defp open(options, tmp, attempts, _) when attempts < @max_attempts do path = path(options, tmp) diff --git a/test/lib/briefly_test.exs b/test/lib/briefly_test.exs index d506aaa..033d475 100644 --- a/test/lib/briefly_test.exs +++ b/test/lib/briefly_test.exs @@ -117,7 +117,7 @@ defmodule Test.Briefly do end) end - test "can create and remove a directory" do + test "can create and remove a directory with the deprecated boolean option" do parent = self() {pid, ref} = @@ -143,6 +143,32 @@ defmodule Test.Briefly do end end + test "can create and remove a directory with the type: :directory option" do + parent = self() + + {pid, ref} = + spawn_monitor(fn -> + {:ok, path} = Briefly.create(type: :directory) + send(parent, {:path, path}) + assert File.stat!(path).type == :directory + File.write!(Path.join(path, "a-file"), "some content") + end) + + path = + receive do + {:path, path} -> path + after + 1_000 -> flunk("didn't get a path") + end + + receive do + {:DOWN, ^ref, :process, ^pid, :normal} -> + # Give the rm_rf a chance to finish + :timer.sleep(1000) + refute File.exists?(path) + end + end + test "terminate removes all files" do {:ok, path} = Briefly.create() :ok = Briefly.Entry.terminate(:shutdown, []) @@ -294,4 +320,39 @@ defmodule Test.Briefly do end end end + + test "returns an io device if device is requested" do + parent = self() + + {pid, ref} = + spawn_monitor(fn -> + {:ok, path1, device1} = Briefly.create(%{type: :device}) + IO.write(device1, @fixture) + assert File.read!(path1) == @fixture + {:ok, path2, device2} = Briefly.create(%{type: :device}) + File.write!(path2, @fixture) + assert IO.read(device2, 1024) == @fixture + send(parent, {:paths_and_devices, [path1, path2], [device1, device2]}) + end) + + {paths, devices} = + receive do + {:paths_and_devices, paths, devices} -> {paths, devices} + after + 1_000 -> flunk("didn't get paths") + end + + receive do + {:DOWN, ^ref, :process, ^pid, :normal} -> + {:ok, _} = Briefly.create() + + Enum.each(paths, fn path -> + refute File.exists?(path) + end) + + Enum.each(devices, fn device -> + refute Process.info(device) + end) + end + end end From 70931cb1c83732c247c369ee4afb9fc0bef91eaf Mon Sep 17 00:00:00 2001 From: Kyle Butt Date: Wed, 18 Oct 2023 12:49:42 -0600 Subject: [PATCH 2/4] Handle the lack of synchronization between monitors in test There is no guarantee that the cleanup will have run before the test thread receives the monitor notification. Busy wait watching ETS to ensure that cleanup has had a chance to run. --- test/lib/briefly_test.exs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/lib/briefly_test.exs b/test/lib/briefly_test.exs index 033d475..bca192b 100644 --- a/test/lib/briefly_test.exs +++ b/test/lib/briefly_test.exs @@ -67,6 +67,7 @@ defmodule Test.Briefly do receive do {:DOWN, ^monitor_ref, :process, ^monitor_pid, :normal} -> {:ok, _} = Briefly.create() + cleanup_wait_loop(monitor_pid) refute File.exists?(path) end end @@ -355,4 +356,15 @@ defmodule Test.Briefly do end) end end + + # There is no synchronization between cleanup and other monitors, so there + # is a race condition where the cleanup may not have happened yet. Use ets + # as a signal that the test can use to busy wait. More likely to occur with + # debugging prints or intentional delays + defp cleanup_wait_loop(pid) do + if :ets.member(Briefly.Entry.Dir, pid) do + Process.sleep(100) + cleanup_wait_loop(pid) + end + end end From 9878f7136ac12a856c68c6086d57c3c72054ed13 Mon Sep 17 00:00:00 2001 From: Kyle Butt Date: Wed, 18 Oct 2023 12:52:51 -0600 Subject: [PATCH 3/4] Add pid_registered? helper to check if a pid is in ets Just hides one ets usage behind a helper. No other functional changes. --- lib/briefly/entry.ex | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/briefly/entry.ex b/lib/briefly/entry.ex index e9f0146..6cf3d40 100644 --- a/lib/briefly/entry.ex +++ b/lib/briefly/entry.ex @@ -57,25 +57,22 @@ defmodule Briefly.Entry do @doc false def give_away(path, to_pid, from_pid) when is_binary(path) and is_pid(to_pid) and is_pid(from_pid) do - with [{^from_pid, _tmp}] <- :ets.lookup(@dir_table, from_pid), + with true <- pid_registered?(from_pid), true <- path_owner?(from_pid, path) do - case :ets.lookup(@dir_table, to_pid) do - [{^to_pid, _tmp}] -> - :ets.insert(@path_table, {to_pid, path}) - :ets.delete_object(@path_table, {from_pid, path}) - - :ok - - [] -> - server = server() + if pid_registered?(to_pid) do + :ets.insert(@path_table, {to_pid, path}) + :ets.delete_object(@path_table, {from_pid, path}) + :ok + else + server = server() - {:ok, tmps} = GenServer.call(server, :roots) - {:ok, tmp} = generate_tmp_dir(tmps) - :ok = GenServer.call(server, {:give_away, to_pid, tmp, path}) + {:ok, tmps} = GenServer.call(server, :roots) + {:ok, tmp} = generate_tmp_dir(tmps) + :ok = GenServer.call(server, {:give_away, to_pid, tmp, path}) - :ets.delete_object(@path_table, {from_pid, path}) + :ets.delete_object(@path_table, {from_pid, path}) - :ok + :ok end else _ -> @@ -255,6 +252,10 @@ defmodule Briefly.Entry do defp extname(%{extname: value}), do: value defp extname(_), do: Briefly.Config.default_extname() + defp pid_registered?(pid) do + :ets.member(@dir_table, pid) + end + defp path_owner?(pid, path) do owned_paths = :ets.lookup(@path_table, pid) Enum.any?(owned_paths, fn {_pid, p} -> p == path end) From 5435cdf4cfcd335278c3f350628334871aa44745 Mon Sep 17 00:00:00 2001 From: Kyle Butt Date: Tue, 17 Oct 2023 13:20:38 -0600 Subject: [PATCH 4/4] Better handling of base temporary directories Create a new temporary directory with defined permissions for each pid. Ensuring that the creation is new prevents race conditions where the directory already exists. Ideally we would pass the mode when creating the directory, but that is not possible with the Elixir nor Erlang standard libraries. When cleaning up for a pid, optimistically attempt to delete the temporary directory using rmdir. This won't delete the directory if any files are still there, which occurs if they have been given away. When cleaning up for a pid, look for any temporary directories for files we have been gifted. Remove them if pid in question is no longer using the temporary directory. Allow the application to configure the mode for the temporary directory. --- README.md | 1 + lib/briefly/entry.ex | 128 +++++++++++++++++++++++++++----------- mix.exs | 1 + test/lib/briefly_test.exs | 125 +++++++++++++++++++++++++++++++++++++ 4 files changed, 217 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index e2e031d..28f3134 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,7 @@ following Mix config: # config/config.exs config :briefly, directory: [{:system, "TMPDIR"}, {:system, "TMP"}, {:system, "TEMP"}, "/tmp"], + directory_mode: 0o755, default_prefix: "briefly", default_extname: "" ``` diff --git a/lib/briefly/entry.ex b/lib/briefly/entry.ex index 6cf3d40..a617c9e 100644 --- a/lib/briefly/entry.ex +++ b/lib/briefly/entry.ex @@ -41,13 +41,34 @@ defmodule Briefly.Entry do @doc false def cleanup(pid) do case :ets.lookup(@dir_table, pid) do - [{^pid, _tmp}] -> + [{^pid, tmp}] -> + path_entries = :ets.lookup(@path_table, pid) + + secondaries = + Enum.reduce( + path_entries, + MapSet.new(), + fn path_entry = {_pid, path_value}, seen -> + delete_path(path_entry) + if path_value.original_owner != pid do + MapSet.put(seen, %{path_value | path: nil}) + else + seen + end + end + ) + + File.rmdir(tmp) + + Enum.each(secondaries, fn %{original_owner: owner_pid, root_dir: dir} -> + if !pid_registered?(owner_pid) do + File.rmdir(dir) + end + end) + + :ets.delete(@path_table, pid) :ets.delete(@dir_table, pid) - - entries = :ets.lookup(@path_table, pid) - Enum.each(entries, &delete_path/1) - - for {_pid, path} <- entries, do: path + for {_pid, path} <- path_entries, do: path [] -> [] @@ -58,19 +79,19 @@ defmodule Briefly.Entry do def give_away(path, to_pid, from_pid) when is_binary(path) and is_pid(to_pid) and is_pid(from_pid) do with true <- pid_registered?(from_pid), - true <- path_owner?(from_pid, path) do + path_entry when not is_nil(path_entry) <- path_entry_if_owner(from_pid, path) do if pid_registered?(to_pid) do - :ets.insert(@path_table, {to_pid, path}) - :ets.delete_object(@path_table, {from_pid, path}) + :ets.insert(@path_table, {to_pid, path_entry}) + :ets.delete_object(@path_table, {from_pid, path_entry}) :ok else server = server() {:ok, tmps} = GenServer.call(server, :roots) {:ok, tmp} = generate_tmp_dir(tmps) - :ok = GenServer.call(server, {:give_away, to_pid, tmp, path}) + :ok = GenServer.call(server, {:give_away, to_pid, tmp, path_entry}) - :ets.delete_object(@path_table, {from_pid, path}) + :ets.delete_object(@path_table, {from_pid, path_entry}) :ok end @@ -101,12 +122,12 @@ defmodule Briefly.Entry do {:reply, {:ok, dirs}, dirs} end - def handle_call({:give_away, pid, tmp, path}, _from, dirs) do + def handle_call({:give_away, pid, tmp, path_entry}, _from, dirs) do # Since we are writing on behalf of another process, we need to make sure # the monitor and writing to the tables happen within the same operation. Process.monitor(pid) :ets.insert_new(@dir_table, {pid, tmp}) - :ets.insert(@path_table, {pid, path}) + :ets.insert(@path_table, {pid, path_entry}) {:reply, :ok, dirs} end @@ -123,6 +144,17 @@ defmodule Briefly.Entry do def terminate(_reason, _state) do folder = fn entry, :ok -> delete_path(entry) end :ets.foldl(folder, :ok, @path_table) + + dir_folder = fn {_key, dir}, :ok -> + case File.rmdir(dir) do + :ok -> :ok + {:error, :eexist} -> :ok + {:error, :enoent} -> :ok + {:error, :enotdir} -> :ok + end + end + + :ets.foldl(dir_folder, :ok, @dir_table) end ## Helpers @@ -146,20 +178,33 @@ defmodule Briefly.Entry do end defp generate_tmp_dir(tmp_roots) do - {mega, _, _} = :os.timestamp() - subdir = "briefly-#{mega}" + subdir = path(%{prefix: "briefly-", extname: ""}) - if tmp = Enum.find_value(tmp_roots, &write_tmp_dir(Path.join(&1, subdir))) do + if tmp = Enum.find_value(tmp_roots, &write_tmp_dir({&1, subdir})) do {:ok, tmp} else {:error, %Briefly.NoRootDirectoryError{tmp_dirs: tmp_roots}} end end - defp write_tmp_dir(path) do - case File.mkdir_p(path) do - :ok -> path - {:error, _} -> nil + defp write_tmp_dir({root, path}) do + fullpath = Path.join(root, path) + + case File.mkdir_p(root) do + {:error, _} -> + nil + + :ok -> + case File.mkdir(fullpath) do + {:error, _} -> + nil + + :ok -> + case File.chmod(fullpath, Briefly.Config.directory_mode()) do + {:error, _} -> nil + :ok -> fullpath + end + end end end @@ -168,7 +213,8 @@ defmodule Briefly.Entry do case File.mkdir_p(path) do :ok -> - :ets.insert(@path_table, {self(), path}) + value = %{path: path, root_dir: tmp, original_owner: self()} + :ets.insert(@path_table, {self(), value}) {:ok, path} {:error, reason} when reason in [:eexist, :eacces] -> @@ -185,7 +231,8 @@ defmodule Briefly.Entry do case File.open(path, [:read, :write, :exclusive]) do {:ok, device_pid} -> - :ets.insert(@path_table, {self(), path}) + value = %{path: path, root_dir: tmp, original_owner: self()} + :ets.insert(@path_table, {self(), value}) {:ok, path, device_pid} {:error, reason} when reason in [:eexist, :eacces] -> @@ -207,7 +254,8 @@ defmodule Briefly.Entry do case :file.write_file(path, "", [:write, :raw, :exclusive, :binary]) do :ok -> - :ets.insert(@path_table, {self(), path}) + value = %{path: path, root_dir: tmp, original_owner: self()} + :ets.insert(@path_table, {self(), value}) {:ok, path} {:error, reason} when reason in [:eexist, :eacces] -> @@ -223,20 +271,21 @@ defmodule Briefly.Entry do {:error, last_error} end - defp path(options, tmp) do + defp path(options) do time = :erlang.monotonic_time() |> to_string |> String.trim("-") - folder = - Enum.join( - [ - prefix(options), - time, - random_padding() - ], - "-" - ) <> extname(options) - - Path.join([tmp, folder]) + Enum.join( + [ + prefix(options), + time, + random_padding() + ], + "-" + ) <> extname(options) + end + + defp path(options, tmp) do + Path.join([tmp, path(options)]) end defp random_padding(length \\ 20) do @@ -256,12 +305,15 @@ defmodule Briefly.Entry do :ets.member(@dir_table, pid) end - defp path_owner?(pid, path) do + defp path_entry_if_owner(pid, path) do owned_paths = :ets.lookup(@path_table, pid) - Enum.any?(owned_paths, fn {_pid, p} -> p == path end) + + Enum.find_value(owned_paths, fn {_pid, %{path: p} = entry} -> + if p == path, do: entry + end) end - defp delete_path({_pid, path}) do + defp delete_path({_pid, %{path: path}}) do File.rm_rf(path) :ok end diff --git a/mix.exs b/mix.exs index 7edfec9..4377b7c 100644 --- a/mix.exs +++ b/mix.exs @@ -56,6 +56,7 @@ defmodule Briefly.Mixfile do defp default_env do [ directory: [{:system, "TMPDIR"}, {:system, "TMP"}, {:system, "TEMP"}, "/tmp"], + directory_mode: 0o755, default_prefix: "briefly", default_extname: "" ] diff --git a/test/lib/briefly_test.exs b/test/lib/briefly_test.exs index bca192b..1554cea 100644 --- a/test/lib/briefly_test.exs +++ b/test/lib/briefly_test.exs @@ -320,6 +320,131 @@ defmodule Test.Briefly do refute File.exists?(path1) end end + + test "cleans up gifted temporary directories when appropriate" do + parent = self() + + {receive1_pid, receive1_ref} = + spawn_monitor(fn -> + receive do + :exit -> nil + end + end) + + {receive2_pid, receive2_ref} = + spawn_monitor(fn -> + receive do + :exit -> nil + end + end) + + {give_pid, give_ref} = + spawn_monitor(fn -> + {:ok, path1} = Briefly.create() + send(parent, {:path1, path1}) + File.open!(path1) + + {:ok, path2} = Briefly.create() + send(parent, {:path2, path2}) + File.open!(path2) + + :ok = Briefly.give_away(path1, receive1_pid) + :ok = Briefly.give_away(path2, receive1_pid) + + send(parent, :given_away) + + receive do + :continue -> :ok + end + + {:ok, path3} = Briefly.create() + send(parent, {:path3, path3}) + File.open!(path3) + + :ok = Briefly.give_away(path3, receive2_pid) + end) + + path1 = + receive do + {:path1, path} -> path + after + 1_000 -> flunk("didn't get a path") + end + + path2 = + receive do + {:path2, path} -> path + after + 1_000 -> flunk("didn't get a path") + end + + receive do + :given_away -> nil + after + 1_000 -> flunk("didn't get signal for given_away") + end + + send(receive1_pid, :exit) + + receive do + {:DOWN, ^receive1_ref, :process, ^receive1_pid, :normal} -> + # force sync by creating file in unknown process + parent = self() + + spawn(fn -> + {:ok, _} = Briefly.create() + send(parent, :continue) + end) + + receive do + :continue -> :ok + end + + cleanup_wait_loop(receive1_pid) + + refute File.exists?(path1) + refute File.exists?(path2) + assert File.exists?(Path.dirname(path1)) + end + + send(give_pid, :continue) + + path3 = + receive do + {:path3, path} -> path + after + 1_000 -> flunk("didn't get a path") + end + + receive do + {:DOWN, ^give_ref, :process, ^give_pid, :normal} -> + {:ok, _} = Briefly.create() + + assert File.exists?(path3) + end + + send(receive2_pid, :exit) + + receive do + {:DOWN, ^receive2_ref, :process, ^receive2_pid, :normal} -> + # force sync by creating file in unknown process + parent = self() + + spawn(fn -> + {:ok, _} = Briefly.create() + send(parent, :continue) + end) + + receive do + :continue -> :ok + end + + cleanup_wait_loop(receive2_pid) + + refute File.exists?(path3) + refute File.exists?(Path.dirname(path3)) + end + end end test "returns an io device if device is requested" do