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

@@ -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: ""
 ```

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

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: ""
     ]

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