[WasmFS] Eagerly read directory entries when opening directories (#19556)

This ensures that subsequent getdents calls see a stable set of entries, even as
entries are removed or inserted after the open. Although on its face this sounds
like it would be incorrect behavior, it is in fact allowed by the spec and
solves a bug where deleting entries while iterating through them would cause
some entries to be skipped.

Fixes #19106.

Author: tlively

.circleci/config.yml

@@ -515,6 +515,7 @@ jobs:
             wasmfs.test_unistd_close
             wasmfs.test_unistd_truncate
             wasmfs.test_readdir
+            wasmfs.test_readdir_unlink
             wasmfs.test_unistd_pipe
             wasmfs.test_fs_write
             wasmfs.test_webidl

system/lib/wasmfs/file_table.cpp

@@ -60,4 +60,30 @@ FileTable::Handle::addEntry(std::shared_ptr<OpenFileState> openFileState) {
   return -EBADF;
 }
 
+int OpenFileState::create(std::shared_ptr<File> file,
+                          oflags_t flags,
+                          std::shared_ptr<OpenFileState>& out) {
+  assert(file);
+  std::vector<Directory::Entry> dirents;
+  if (auto f = file->dynCast<DataFile>()) {
+    if (int err = f->locked().open(flags & O_ACCMODE)) {
+      return err;
+    }
+  } else if (auto d = file->dynCast<Directory>()) {
+    // We are opening a directory; cache its contents for subsequent reads.
+    auto lockedDir = d->locked();
+    dirents = {{".", File::DirectoryKind, d->getIno()},
+               {"..", File::DirectoryKind, lockedDir.getParent()->getIno()}};
+    auto entries = lockedDir.getEntries();
+    if (int err = entries.getError()) {
+      return err;
+    }
+    dirents.insert(dirents.end(), entries->begin(), entries->end());
+  }
+
+  out = std::make_shared<OpenFileState>(
+    private_key{0}, flags, file, std::move(dirents));
+  return 0;
+}
+
 } // namespace wasmfs

system/lib/wasmfs/file_table.h

@@ -58,21 +58,24 @@ class OpenFileState : public std::enable_shared_from_this<OpenFileState> {
   friend FileTable;
 
 public:
-  OpenFileState(private_key, oflags_t flags, std::shared_ptr<File> file)
-    : file(file), flags(flags) {}
+  // Cache directory entries at the moment the directory is opened so that
+  // subsequent getdents calls have a stable view of the contents. Including
+  // files removed after the open and excluding files added after the open is
+  // allowed, and trying to recalculate the directory contents on each getdents
+  // call could lead to missed directory entries if there are concurrent
+  // deletions that effectively move entries back past the current read position
+  // in the open directory.
+  const std::vector<Directory::Entry> dirents;
+
+  OpenFileState(private_key,
+                oflags_t flags,
+                std::shared_ptr<File> file,
+                std::vector<Directory::Entry>&& dirents)
+    : file(file), flags(flags), dirents(std::move(dirents)) {}
 
   [[nodiscard]] static int create(std::shared_ptr<File> file,
                                   oflags_t flags,
-                                  std::shared_ptr<OpenFileState>& out) {
-    assert(file);
-    if (auto f = file->dynCast<DataFile>()) {
-      if (int err = f->locked().open(flags & O_ACCMODE)) {
-        return err;
-      }
-    }
-    out = std::make_shared<OpenFileState>(private_key{0}, flags, file);
-    return 0;
-  }
+                                  std::shared_ptr<OpenFileState>& out);
 
   class Handle {
     std::shared_ptr<OpenFileState> openFileState;

system/lib/wasmfs/syscalls.cpp

@@ -888,19 +888,11 @@ int __syscall_getdents64(int fd, intptr_t dirp, size_t count) {
     return 0;
   }
 
-  std::vector<Directory::Entry> entries = {
-    {".", File::DirectoryKind, dir->getIno()},
-    {"..", File::DirectoryKind, parent->getIno()}};
-  auto dirEntries = lockedDir.getEntries();
-  if (int err = dirEntries.getError()) {
-    return err;
-  }
-  entries.insert(entries.end(), dirEntries->begin(), dirEntries->end());
-
   off_t bytesRead = 0;
-  for (; index < entries.size() && bytesRead + sizeof(dirent) <= count;
+  const auto& dirents = openFile->dirents;
+  for (; index < dirents.size() && bytesRead + sizeof(dirent) <= count;
        index++) {
-    auto& entry = entries[index];
+    const auto& entry = dirents[index];
     result->d_ino = entry.ino;
     result->d_off = index + 1;
     result->d_reclen = sizeof(dirent);

test/dirent/test_readdir_unlink.c

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2023 The Emscripten Authors.  All rights reserved.
+ * Emscripten is available under two separate licenses, the MIT license and the
+ * University of Illinois/NCSA Open Source License.  Both these licenses can be
+ * found in the LICENSE file.
+ */
+
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int main() {
+  if (mkdir("test", 0777) != 0) {
+    printf("Unable to create dir 'test'\n");
+    return 1;
+  }
+
+  for (int i = 0; i < 10; i++) {
+    char path[10];
+    snprintf(path, 10, "test/%d", i);
+    int fd = open(path, O_CREAT, 0777);
+    if (fd < 0) {
+      printf("Unable to create file '%s'\n", path);
+      return 1;
+    }
+    close(fd);
+  }
+
+  DIR* dir = opendir("test");
+  if (!dir) {
+    printf("Unable to open dir 'test'\n");
+    return 1;
+  }
+
+  struct dirent* dirent;
+
+  while ((dirent = readdir(dir)) != NULL) {
+    if (strcmp(dirent->d_name, ".") == 0 || strcmp(dirent->d_name, "..") == 0) {
+      continue;
+    }
+    char path[10];
+    snprintf(path, 10, "test/%s", dirent->d_name);
+    if (unlink(path)) {
+      printf("Unable to unlink '%s'\n", path);
+      return 1;
+    }
+    printf("Unlinked '%s'\n", path);
+
+    // Add a new file in the middle. This will not be visited (although it would
+    // be valid to visit it).
+    if (strcmp(dirent->d_name, "5") == 0) {
+      int fd = open("test/X", O_CREAT, 0777);
+      if (fd < 0) {
+        printf("Unable to create file 'test/X'\n");
+        return 1;
+      }
+      close(fd);
+    }
+  }
+
+  closedir(dir);
+
+  // The new file should still exist.
+  if (access("test/X", F_OK) != 0) {
+    printf("Expected file 'test/X' to exist\n");
+    return 1;
+  }
+  if (unlink("test/X")) {
+    printf("Unable to unlink 'test/X'\n");
+    return 1;
+  }
+
+  if (rmdir("test")) {
+    printf("Unable to remove dir 'test'\n");
+    return 1;
+  }
+
+  printf("success\n");
+
+  return 0;
+}

test/dirent/test_readdir_unlink.out

@@ -0,0 +1,11 @@
+Unlinked 'test/0'
+Unlinked 'test/1'
+Unlinked 'test/2'
+Unlinked 'test/3'
+Unlinked 'test/4'
+Unlinked 'test/5'
+Unlinked 'test/6'
+Unlinked 'test/7'
+Unlinked 'test/8'
+Unlinked 'test/9'
+success

test/other/metadce/test_metadce_files_wasmfs.funcs

@@ -1,4 +1,3 @@
-$__cxa_pure_virtual
 $__cxx_global_array_dtor
 $__cxx_global_array_dtor.1
 $__cxx_global_array_dtor.2
@@ -15,7 +14,6 @@ $__cxxabiv1::__class_type_info::process_found_base_class\28__cxxabiv1::__dynamic
 $__cxxabiv1::__class_type_info::process_static_type_above_dst\28__cxxabiv1::__dynamic_cast_info*\2c\20void\20const*\2c\20void\20const*\2c\20int\29\20const
 $__cxxabiv1::__class_type_info::search_above_dst\28__cxxabiv1::__dynamic_cast_info*\2c\20void\20const*\2c\20void\20const*\2c\20int\2c\20bool\29\20const
 $__cxxabiv1::__class_type_info::search_below_dst\28__cxxabiv1::__dynamic_cast_info*\2c\20void\20const*\2c\20int\2c\20bool\29\20const
-$__cxxabiv1::__class_type_info::~__class_type_info\28\29
 $__cxxabiv1::__si_class_type_info::has_unambiguous_public_base\28__cxxabiv1::__dynamic_cast_info*\2c\20void*\2c\20int\29\20const
 $__cxxabiv1::__si_class_type_info::search_above_dst\28__cxxabiv1::__dynamic_cast_info*\2c\20void\20const*\2c\20void\20const*\2c\20int\2c\20bool\29\20const
 $__cxxabiv1::__si_class_type_info::search_below_dst\28__cxxabiv1::__dynamic_cast_info*\2c\20void\20const*\2c\20int\2c\20bool\29\20const
@@ -34,6 +32,7 @@ $__unlockfile
 $__wasi_fd_close
 $__wasi_fd_write
 $__wasm_call_ctors
+$decltype\28auto\29\20std::__2::__variant_detail::__visitation::__base::__dispatcher<0ul>::__dispatch\5babi:v160004\5d<std::__2::__variant_detail::__dtor<std::__2::__variant_detail::__traits<std::__2::vector<wasmfs::Directory::Entry\2c\20std::__2::allocator<wasmfs::Directory::Entry>>\2c\20int>\2c\20\28std::__2::__variant_detail::_Trait\291>::__destroy\5babi:v160004\5d\28\29::'lambda'\28auto&\29&&\2c\20std::__2::__variant_detail::__base<\28std::__2::__variant_detail::_Trait\291\2c\20std::__2::vector<wasmfs::Directory::Entry\2c\20std::__2::allocator<wasmfs::Directory::Entry>>\2c\20int>&>\28auto\2c\20std::__2::__variant_detail::__base<\28std::__2::__variant_detail::_Trait\291\2c\20std::__2::vector<wasmfs::Directory::Entry\2c\20std::__2::allocator<wasmfs::Directory::Entry>>\2c\20int>&\29
 $decltype\28auto\29\20std::__2::__variant_detail::__visitation::__base::__dispatcher<1ul>::__dispatch\5babi:v160004\5d<std::__2::__variant_detail::__dtor<std::__2::__variant_detail::__traits<long\2c\20std::__2::shared_ptr<wasmfs::File>>\2c\20\28std::__2::__variant_detail::_Trait\291>::__destroy\5babi:v160004\5d\28\29::'lambda'\28auto&\29&&\2c\20std::__2::__variant_detail::__base<\28std::__2::__variant_detail::_Trait\291\2c\20long\2c\20std::__2::shared_ptr<wasmfs::File>>&>\28auto\2c\20std::__2::__variant_detail::__base<\28std::__2::__variant_detail::_Trait\291\2c\20long\2c\20std::__2::shared_ptr<wasmfs::File>>&\29
 $dlfree
 $dlmalloc
@@ -46,6 +45,7 @@ $memmove
 $operator\20new\28unsigned\20long\29
 $pthread_mutex_init
 $sbrk
+$std::__2::__allocation_result<std::__2::allocator_traits<std::__2::allocator<char>>::pointer>\20std::__2::__allocate_at_least\5babi:v160004\5d<std::__2::allocator<char>>\28std::__2::allocator<char>&\2c\20unsigned\20long\29
 $std::__2::__shared_ptr_emplace<wasmfs::MemoryDataFile\2c\20std::__2::allocator<wasmfs::MemoryDataFile>>::~__shared_ptr_emplace\28\29
 $std::__2::__shared_ptr_emplace<wasmfs::MemoryDataFile\2c\20std::__2::allocator<wasmfs::MemoryDataFile>>::~__shared_ptr_emplace\28\29.1
 $std::__2::__shared_ptr_emplace<wasmfs::MemoryDirectory\2c\20std::__2::allocator<wasmfs::MemoryDirectory>>::__on_zero_shared\28\29
@@ -71,12 +71,18 @@ $std::__2::__shared_ptr_emplace<wasmfs::SpecialFiles::\28anonymous\20namespace\2
 $std::__2::__shared_weak_count::__release_weak\28\29
 $std::__2::__shared_weak_count::lock\28\29
 $std::__2::__tree<std::__2::__value_type<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>\2c\20wasmfs::Directory::DCacheEntry>\2c\20std::__2::__map_value_compare<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>\2c\20std::__2::__value_type<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>\2c\20wasmfs::Directory::DCacheEntry>\2c\20std::__2::less<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>>\2c\20true>\2c\20std::__2::allocator<std::__2::__value_type<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>\2c\20wasmfs::Directory::DCacheEntry>>>::destroy\28std::__2::__tree_node<std::__2::__value_type<std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>\2c\20wasmfs::Directory::DCacheEntry>\2c\20void*>*\29
-$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__begin_lifetime\5babi:v160004\5d\28char*\2c\20unsigned\20long\29
+$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>&\20std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__assign_no_alias<false>\28char\20const*\2c\20unsigned\20long\29
+$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>&\20std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__assign_no_alias<true>\28char\20const*\2c\20unsigned\20long\29
+$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__grow_by_and_replace\28unsigned\20long\2c\20unsigned\20long\2c\20unsigned\20long\2c\20unsigned\20long\2c\20unsigned\20long\2c\20unsigned\20long\2c\20char\20const*\29
 $std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__init_copy_ctor_external\28char\20const*\2c\20unsigned\20long\29
+$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__invalidate_iterators_past\5babi:v160004\5d\28unsigned\20long\29
+$std::__2::basic_string<char\2c\20std::__2::char_traits<char>\2c\20std::__2::allocator<char>>::__throw_length_error\5babi:v160004\5d\28\29\20const
+$std::__2::char_traits<char>::copy\28char*\2c\20char\20const*\2c\20unsigned\20long\29
 $std::__2::shared_ptr<wasmfs::SpecialFiles::\28anonymous\20namespace\29::RandomFile>\20std::__2::make_shared\5babi:v160004\5d<wasmfs::SpecialFiles::\28anonymous\20namespace\29::RandomFile\2c\20void>\28\29
 $std::__2::vector<unsigned\20char\2c\20std::__2::allocator<unsigned\20char>>::__append\28unsigned\20long\29
 $std::__2::vector<wasmfs::MemoryDirectory::ChildEntry\2c\20std::__2::allocator<wasmfs::MemoryDirectory::ChildEntry>>::erase\5babi:v160004\5d\28std::__2::__wrap_iter<wasmfs::MemoryDirectory::ChildEntry\20const*>\29
 $strlen
+$void\20std::__2::__libcpp_operator_delete\5babi:v160004\5d<void*>\28void*\29
 $void\20std::__2::vector<std::__2::shared_ptr<wasmfs::OpenFileState>\2c\20std::__2::allocator<std::__2::shared_ptr<wasmfs::OpenFileState>>>::__emplace_back_slow_path<>\28\29
 $wasmfs::DataFile::Handle::flush\28\29
 $wasmfs::DataFile::Handle::write\28unsigned\20char\20const*\2c\20unsigned\20long\2c\20long\20long\29

test/other/metadce/test_metadce_files_wasmfs.size

@@ -1 +1 @@
-47462
+51960

test/test_core.py

@@ -5814,6 +5814,9 @@ def test_readdir(self):
   def test_readdir_empty(self):
     self.do_run_in_out_file_test('dirent/test_readdir_empty.c')
 
+  def test_readdir_unlink(self):
+    self.do_run_in_out_file_test('dirent/test_readdir_unlink.c')
+
   def test_stat(self):
     self.set_setting("FORCE_FILESYSTEM")
     self.do_runf(test_file('stat/test_stat.c'), 'success')