From 5435cdf4cfcd335278c3f350628334871aa44745 Mon Sep 17 00:00:00 2001 From: Kyle Butt Date: Tue, 17 Oct 2023 13:20:38 -0600 Subject: [PATCH] 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