From 52f11baac07da5cdf93c362db72dbf11a5298c51 Mon Sep 17 00:00:00 2001 From: Shanee Date: Tue, 8 Sep 2026 09:43:23 +0100 Subject: [PATCH] WasmFS: take directory locks parent-first in renameat __syscall_renameat locked both parent directories and then walked the new parent's ancestors, locking each on the way up: child before parent. Every path lookup locks parent before child, so two threads in one directory tree deadlocked, and every later rename in the process blocked behind them on renameMutex. A destination directly under the root never hit it because the walk exits before taking a lock. Run the ancestor walk before either parent is locked, one lock at a time (renames are serialized and only a rename re-parents a directory, so the chain cannot change under the walk), then lock the two parents ancestor-first. Fixes #27684. --- ChangeLog.md | 2 + system/lib/wasmfs/syscalls.cpp | 59 ++++++++++++++++---- test/test_other.py | 7 +++ test/wasmfs/wasmfs_rename_race.c | 90 ++++++++++++++++++++++++++++++ test/wasmfs/wasmfs_rename_race.out | 1 + 5 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 test/wasmfs/wasmfs_rename_race.c create mode 100644 test/wasmfs/wasmfs_rename_race.out diff --git a/ChangeLog.md b/ChangeLog.md index e09084933ccd9..d2ad722c26986 100644 --- a/ChangeLog.md +++ b/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 diff --git a/system/lib/wasmfs/syscalls.cpp b/system/lib/wasmfs/syscalls.cpp index 8d098b12d2213..643df1340130b 100644 --- a/system/lib/wasmfs/syscalls.cpp +++ b/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 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()) { diff --git a/test/test_other.py b/test/test_other.py index c8e595b889f7f..5f66225d05407 100644 --- a/test/test_other.py +++ b/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']) diff --git a/test/wasmfs/wasmfs_rename_race.c b/test/wasmfs/wasmfs_rename_race.c new file mode 100644 index 0000000000000..a17fd1bab5ee8 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include +#include +#include + +#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; +} diff --git a/test/wasmfs/wasmfs_rename_race.out b/test/wasmfs/wasmfs_rename_race.out new file mode 100644 index 0000000000000..9766475a4185a --- /dev/null +++ b/test/wasmfs/wasmfs_rename_race.out @@ -0,0 +1 @@ +ok