WasmFS: take directory locks parent-first in renameat (fixes rename deadlock)

ChangeLog.md

@@ -22,6 +22,8 @@ See docs/process.md for more on how version tagging works.
 ----------------------
 - The SDL3 port is no longer considered experimental, and the compiler
   diagnostic warning has been removed. (#27646)
+- WasmFS no longer deadlocks when a `rename` runs concurrently with path
+  lookups in the same directory tree. (#27684)
 - `WASM=0` and `WASM=2` (wasm2js) were marked as deprecated. (See #27608)
 - mimalloc was updated to 3.5.1. (#27662)
 - `-sWASM_BINDGEN` supports emcc usage as a post-link step, where

system/lib/wasmfs/syscalls.cpp

@@ -1018,9 +1018,54 @@ int __syscall_renameat(int olddirfd,
     return -ENAMETOOLONG;
   }
 
-  // Lock both directories.
-  auto lockedOldParent = oldParent->locked();
-  auto lockedNewParent = newParent->locked();
+  auto root = wasmFS.getRootDirectory();
+
+  // Every other path operation takes directory locks parent-before-child, so
+  // this function must never hold a directory's lock while acquiring an
+  // ancestor's. The ancestor walk below therefore runs before either parent
+  // is locked, holding one lock at a time, and the two parent locks are then
+  // taken ancestor-first. Renames are serialized above and nothing else
+  // re-parents a directory, so the ancestry cannot change under the walk.
+  std::shared_ptr<File> oldFileForWalk;
+  {
+    auto lockedOldParent = oldParent->locked();
+    oldFileForWalk = lockedOldParent.getChild(oldFileName);
+  }
+  if (!oldFileForWalk) {
+    return -ENOENT;
+  }
+  if (oldFileForWalk == root) {
+    return -EBUSY;
+  }
+
+  // Check that oldDir is not an ancestor of newDir, and whether oldParent is
+  // an ancestor of newParent.
+  bool oldParentAboveNew = false;
+  for (auto curr = newParent; curr && curr != root;
+       curr = curr->locked().getParent()) {
+    if (curr == oldFileForWalk) {
+      return -EINVAL;
+    }
+    if (curr == oldParent) {
+      oldParentAboveNew = true;
+    }
+  }
+  bool newParentAboveOld = false;
+  if (!oldParentAboveNew) {
+    for (auto curr = oldParent; curr && curr != root;
+         curr = curr->locked().getParent()) {
+      if (curr == newParent) {
+        newParentAboveOld = true;
+        break;
+      }
+    }
+  }
+
+  // Lock both directories, ancestor first.
+  auto lockedFirst = (newParentAboveOld ? newParent : oldParent)->locked();
+  auto lockedSecond = (newParentAboveOld ? oldParent : newParent)->locked();
+  auto& lockedOldParent = newParentAboveOld ? lockedSecond : lockedFirst;
+  auto& lockedNewParent = newParentAboveOld ? lockedFirst : lockedSecond;
 
   // Get the source and destination files.
   auto oldFile = lockedOldParent.getChild(oldFileName);
@@ -1036,7 +1081,6 @@ int __syscall_renameat(int olddirfd,
   }
 
   // Never allow renaming or overwriting the root.
-  auto root = wasmFS.getRootDirectory();
   if (oldFile == root || newFile == root) {
     return -EBUSY;
   }
@@ -1052,13 +1096,6 @@ int __syscall_renameat(int olddirfd,
     return -EXDEV;
   }
 
-  // Check that oldDir is not an ancestor of newDir.
-  for (auto curr = newParent; curr != root; curr = curr->locked().getParent()) {
-    if (curr == oldFile) {
-      return -EINVAL;
-    }
-  }
-
   // The new file will be removed if it already exists.
   if (newFile) {
     if (auto newDir = newFile->dynCast<Directory>()) {

test/test_other.py

@@ -13841,6 +13841,13 @@ def test_wasmfs_before_preload(self):
     create_file('js_backend_files/file.dat', 'data')
     self.do_runf_out_file('wasmfs/wasmfs_before_preload.c', cflags=['--preload-file', 'js_backend_files/file.dat'])
 
+  @requires_pthreads
+  def test_wasmfs_rename_race(self):
+    self.set_setting('WASMFS')
+    self.set_setting('EXIT_RUNTIME')
+    self.set_setting('PTHREAD_POOL_SIZE', 8)
+    self.do_runf_out_file('wasmfs/wasmfs_rename_race.c')
+
   def test_hello_world_above_2gb(self):
     self.do_runf_out_file('hello_world.c', cflags=['-sGLOBAL_BASE=2GB', '-sINITIAL_MEMORY=3GB'])
 

test/wasmfs/wasmfs_rename_race.c

@@ -0,0 +1,90 @@
+// Concurrent renames, moves between a directory and its parent, and path
+// lookups in the same directory tree must not deadlock.
+#include <assert.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#define ITERATIONS 200
+
+static void write_file(const char* path) {
+  int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+  assert(fd >= 0);
+  assert(write(fd, "x", 1) == 1);
+  assert(close(fd) == 0);
+}
+
+// Publish files by writing a temporary and renaming it into place in the same
+// directory, two levels below the root.
+static void* publisher(void* arg) {
+  int id = (int)(intptr_t)arg;
+  char tmp[64], final[64];
+  for (int i = 0; i < ITERATIONS; i++) {
+    snprintf(tmp, sizeof(tmp), "/a/b/c/tmp%d_%d", id, i);
+    snprintf(final, sizeof(final), "/a/b/c/file%d_%d", id, i);
+    write_file(tmp);
+    assert(rename(tmp, final) == 0);
+    assert(unlink(final) == 0);
+  }
+  return NULL;
+}
+
+// Move files between a directory and its parent, so the two directories a
+// rename locks are an ancestor and a descendant.
+static void* mover(void* arg) {
+  int id = (int)(intptr_t)arg;
+  char lower[64], upper[64];
+  for (int i = 0; i < ITERATIONS; i++) {
+    snprintf(lower, sizeof(lower), "/a/b/c/move%d_%d", id, i);
+    snprintf(upper, sizeof(upper), "/a/b/move%d_%d", id, i);
+    write_file(lower);
+    assert(rename(lower, upper) == 0);
+    assert(rename(upper, lower) == 0);
+    assert(unlink(lower) == 0);
+  }
+  return NULL;
+}
+
+// Resolve paths through the same tree, which locks each directory on the way
+// down.
+static void* walker(void* arg) {
+  struct stat st;
+  for (int i = 0; i < ITERATIONS * 4; i++) {
+    stat("/a/b/c/absent", &st);
+    stat("/a/b/c", &st);
+    DIR* dir = opendir("/a/b/c");
+    assert(dir);
+    while (readdir(dir)) {
+    }
+    closedir(dir);
+  }
+  return NULL;
+}
+
+int main() {
+  assert(mkdir("/a", 0777) == 0);
+  assert(mkdir("/a/b", 0777) == 0);
+  assert(mkdir("/a/b/c", 0777) == 0);
+
+  pthread_t threads[8];
+  int count = 0;
+  for (int i = 0; i < 3; i++) {
+    assert(pthread_create(&threads[count++], NULL, publisher, (void*)(intptr_t)i) == 0);
+  }
+  for (int i = 0; i < 2; i++) {
+    assert(pthread_create(&threads[count++], NULL, mover, (void*)(intptr_t)i) == 0);
+  }
+  for (int i = 0; i < 3; i++) {
+    assert(pthread_create(&threads[count++], NULL, walker, NULL) == 0);
+  }
+  for (int i = 0; i < count; i++) {
+    assert(pthread_join(threads[i], NULL) == 0);
+  }
+  printf("ok\n");
+  return 0;
+}

test/wasmfs/wasmfs_rename_race.out

@@ -0,0 +1 @@
+ok