[WasmFS] Support errors in open (#17536)

tlively · web-flow · commit 85793a252ae2 · 2022-08-01T12:17:25.000-07:00
Change the signature of the `open` method in the backend API to support
returning error codes. Also refactor how this method is called since it was
previously called in the `OpenFileState` constructor, but constructors are not
fallible.
diff --git a/src/library_wasmfs_opfs.js b/src/library_wasmfs_opfs.js
@@ -232,7 +232,9 @@ mergeInto(LibraryManager.library, {
       }
       accessID = wasmfsOPFSAllocate(wasmfsOPFSAccessHandles, accessHandle);
     } catch (e) {
-      if (e.name === "InvalidStateError") {
+      // TODO: Presumably only one of these will appear in the final API?
+      if (e.name === "InvalidStateError" ||
+          e.name === "NoModificationAllowedError") {
         accessID = -1;
       } else {
         throw e;
diff --git a/system/lib/wasmfs/backends/node_backend.cpp b/system/lib/wasmfs/backends/node_backend.cpp
@@ -156,10 +156,7 @@ class NodeFile : public DataFile {
     WASMFS_UNREACHABLE("TODO: implement NodeFile::setSize");
   }
 
-  void open(oflags_t flags) override {
-    // TODO: Properly report errors.
-    state.open(flags);
-  }
+  int open(oflags_t flags) override { return state.open(flags); }
 
   void close() override { state.close(); }
 
diff --git a/system/lib/wasmfs/backends/opfs_backend.cpp b/system/lib/wasmfs/backends/opfs_backend.cpp
@@ -272,12 +272,7 @@ class OPFSFile : public DataFile {
     }
   }
 
-  void open(oflags_t flags) override {
-    if (auto err = state.open(proxy, fileID, flags); err < 0) {
-      // TODO: Proper error reporting.
-      assert(false && "error during open");
-    }
-  }
+  int open(oflags_t flags) override { return state.open(proxy, fileID, flags); }
 
   void close() override { state.close(proxy); }
 
diff --git a/system/lib/wasmfs/backends/proxied_file_backend.cpp b/system/lib/wasmfs/backends/proxied_file_backend.cpp
@@ -22,8 +22,10 @@ class ProxiedFile : public DataFile {
   emscripten::ProxyWorker& proxy;
   std::shared_ptr<DataFile> baseFile;
 
-  void open(oflags_t flags) override {
-    proxy([&]() { baseFile->locked().open(flags); });
+  int open(oflags_t flags) override {
+    int err;
+    proxy([&]() { err = baseFile->locked().open(flags); });
+    return err;
   }
 
   void close() override {
diff --git a/system/lib/wasmfs/file.h b/system/lib/wasmfs/file.h
@@ -129,9 +129,8 @@ class DataFile : public File {
 protected:
   // Notify the backend when this file is opened or closed. The backend is
   // responsible for keeping files accessible as long as they are open, even if
-  // they are unlinked.
-  // TODO: Report errors.
-  virtual void open(oflags_t flags) = 0;
+  // they are unlinked. Returns 0 on success or a negative error code.
+  virtual int open(oflags_t flags) = 0;
   virtual void close() = 0;
 
   // Return the accessed length or a negative error code. It is not an error to
@@ -289,7 +288,7 @@ class DataFile::Handle : public File::Handle {
   Handle(std::shared_ptr<File> dataFile) : File::Handle(dataFile) {}
   Handle(Handle&&) = default;
 
-  void open(oflags_t flags) { getFile()->open(flags); }
+  [[nodiscard]] int open(oflags_t flags) { return getFile()->open(flags); }
   void close() { getFile()->close(); }
 
   ssize_t read(uint8_t* buf, size_t len, off_t offset) {
diff --git a/system/lib/wasmfs/file_table.cpp b/system/lib/wasmfs/file_table.cpp
@@ -12,12 +12,15 @@
 namespace wasmfs {
 
 FileTable::FileTable() {
-  entries.push_back(
-    std::make_shared<OpenFileState>(0, O_RDONLY, SpecialFiles::getStdin()));
-  entries.push_back(
-    std::make_shared<OpenFileState>(0, O_WRONLY, SpecialFiles::getStdout()));
-  entries.push_back(
-    std::make_shared<OpenFileState>(0, O_WRONLY, SpecialFiles::getStderr()));
+  entries.emplace_back();
+  (void)OpenFileState::create(
+    SpecialFiles::getStdin(), O_RDONLY, entries.back());
+  entries.emplace_back();
+  (void)OpenFileState::create(
+    SpecialFiles::getStdout(), O_WRONLY, entries.back());
+  entries.emplace_back();
+  (void)OpenFileState::create(
+    SpecialFiles::getStderr(), O_WRONLY, entries.back());
 }
 
 std::shared_ptr<OpenFileState> FileTable::Handle::getEntry(__wasi_fd_t fd) {
diff --git a/system/lib/wasmfs/file_table.h b/system/lib/wasmfs/file_table.h
@@ -32,25 +32,40 @@ template<typename T> bool addWillOverFlow(T a, T b) {
 
 class OpenFileState : public std::enable_shared_from_this<OpenFileState> {
   std::shared_ptr<File> file;
-  off_t position;
+  off_t position = 0;
   oflags_t flags; // RD_ONLY, WR_ONLY, RDWR
+
   // An OpenFileState needs a mutex if there are concurrent accesses on one open
   // file descriptor. This could occur if there are multiple seeks on the same
   // open file descriptor.
   std::recursive_mutex mutex;
 
+  // We can't make the constructor private because std::make_shared needs to be
+  // able to call it, but we can make it unusable publicly.
+  struct private_key {
+    explicit private_key(int) {}
+  };
+
 public:
-  OpenFileState(size_t position, oflags_t flags, std::shared_ptr<File> file)
-    : position(position), flags(flags), file(file) {
+  OpenFileState(private_key, oflags_t flags, std::shared_ptr<File> file)
+    : flags(flags), file(file) {}
+
+  ~OpenFileState() {
     if (auto f = file->dynCast<DataFile>()) {
-      f->locked().open(flags & O_ACCMODE);
+      f->locked().close();
     }
   }
 
-  ~OpenFileState() {
+  [[nodiscard]] static int create(std::shared_ptr<File> file,
+                                  oflags_t flags,
+                                  std::shared_ptr<OpenFileState>& out) {
     if (auto f = file->dynCast<DataFile>()) {
-      f->locked().close();
+      if (int err = f->locked().open(flags & O_ACCMODE)) {
+        return err;
+      }
     }
+    out = std::make_shared<OpenFileState>(private_key{0}, flags, file);
+    return 0;
   }
 
   class Handle {
diff --git a/system/lib/wasmfs/js_impl_backend.h b/system/lib/wasmfs/js_impl_backend.h
@@ -77,7 +77,7 @@ class JSImplFile : public DataFile {
   }
 
   // TODO: Notify the JS about open and close events?
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
diff --git a/system/lib/wasmfs/memory_backend.h b/system/lib/wasmfs/memory_backend.h
@@ -19,7 +19,7 @@ namespace wasmfs {
 class MemoryFile : public DataFile {
   std::vector<uint8_t> buffer;
 
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override;
   ssize_t read(uint8_t* buf, size_t len, off_t offset) override;
diff --git a/system/lib/wasmfs/pipe_backend.h b/system/lib/wasmfs/pipe_backend.h
@@ -23,7 +23,7 @@ using PipeData = std::queue<uint8_t>;
 class PipeFile : public DataFile {
   std::shared_ptr<PipeData> data;
 
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
diff --git a/system/lib/wasmfs/proxied_async_js_impl_backend.h b/system/lib/wasmfs/proxied_async_js_impl_backend.h
@@ -85,7 +85,7 @@ class ProxiedAsyncJSImplFile : public DataFile {
   }
 
   // TODO: Notify the JS about open and close events?
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
diff --git a/system/lib/wasmfs/special_files.cpp b/system/lib/wasmfs/special_files.cpp
@@ -19,7 +19,7 @@ namespace {
 
 // No-op reads and writes: /dev/null
 class NullFile : public DataFile {
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
@@ -37,7 +37,7 @@ class NullFile : public DataFile {
 };
 
 class StdinFile : public DataFile {
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
@@ -62,7 +62,7 @@ class WritingStdFile : public DataFile {
 protected:
   std::vector<char> writeBuffer;
 
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t read(uint8_t* buf, size_t len, off_t offset) override {
@@ -134,7 +134,7 @@ class StderrFile : public WritingStdFile {
 };
 
 class RandomFile : public DataFile {
-  void open(oflags_t) override {}
+  int open(oflags_t) override { return 0; }
   void close() override {}
 
   ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {
diff --git a/system/lib/wasmfs/syscalls.cpp b/system/lib/wasmfs/syscalls.cpp
@@ -439,7 +439,10 @@ static __wasi_fd_t doOpen(path::ParsedParent parsed,
         return 0;
       }
 
-      auto openFile = std::make_shared<OpenFileState>(0, flags, created);
+      std::shared_ptr<OpenFileState> openFile;
+      if (auto err = OpenFileState::create(created, flags, openFile)) {
+        return err;
+      }
       return wasmFS.getFileTable().locked().addEntry(openFile);
     }
   }
@@ -486,7 +489,10 @@ static __wasi_fd_t doOpen(path::ParsedParent parsed,
 
   // Note that we open the file before truncating it because some backends may
   // truncate opened files more efficiently (e.g. OPFS).
-  auto openFile = std::make_shared<OpenFileState>(0, flags, child);
+  std::shared_ptr<OpenFileState> openFile;
+  if (auto err = OpenFileState::create(child, flags, openFile)) {
+    return err;
+  }
 
   // If O_TRUNC, truncate the file if possible.
   if (flags & O_TRUNC) {
@@ -1251,10 +1257,12 @@ int __syscall_pipe(intptr_t fd) {
   auto writer = std::make_shared<PipeFile>(S_IWUGO, data);
 
   auto fileTable = wasmFS.getFileTable().locked();
-  fds[0] =
-    fileTable.addEntry(std::make_shared<OpenFileState>(0, O_RDONLY, reader));
-  fds[1] =
-    fileTable.addEntry(std::make_shared<OpenFileState>(0, O_WRONLY, writer));
+
+  std::shared_ptr<OpenFileState> openReader, openWriter;
+  (void)OpenFileState::create(reader, O_RDONLY, openReader);
+  (void)OpenFileState::create(writer, O_WRONLY, openWriter);
+  fds[0] = fileTable.addEntry(openReader);
+  fds[1] = fileTable.addEntry(openWriter);
 
   return 0;
 }
diff --git a/test/test_browser.py b/test/test_browser.py
@@ -5310,6 +5310,14 @@ def test_wasmfs_opfs(self):
     self.btest_exit(test, args=args + ['-DWASMFS_SETUP'])
     self.btest_exit(test, args=args + ['-DWASMFS_RESUME'])
 
+  @requires_threads
+  @no_firefox('no OPFS support yet')
+  def test_wasmfs_opfs_errors(self):
+    test = test_file('wasmfs/wasmfs_opfs_errors.c')
+    postjs = test_file('wasmfs/wasmfs_opfs_errors_post.js')
+    args = ['-sWASMFS', '-pthread', '-sPROXY_TO_PTHREAD', '--post-js', postjs]
+    self.btest(test, args=args, expected="0")
+
   @no_firefox('no 4GB support yet')
   def test_zzz_zzz_emmalloc_memgrowth(self, *args):
     self.btest(test_file('browser/emmalloc_memgrowth.cpp'), expected='0', args=['-sMALLOC=emmalloc', '-sALLOW_MEMORY_GROWTH=1', '-sABORTING_MALLOC=0', '-sASSERTIONS=2', '-sMINIMAL_RUNTIME=1', '-sMAXIMUM_MEMORY=4GB'])
diff --git a/test/wasmfs/wasmfs_opfs_errors.c b/test/wasmfs/wasmfs_opfs_errors.c
@@ -0,0 +1,62 @@
+#include <emscripten/emscripten.h>
+#include <emscripten/wasmfs.h>
+#include <emscripten/console.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+
+int main() {
+  wasmfs_create_directory("/opfs", 0777, wasmfs_create_opfs_backend());
+  EM_ASM({ run_test(); });
+  emscripten_exit_with_live_runtime();
+}
+
+const char* file = "/opfs/data";
+
+// Each of these functions returns:
+//   0: failure with EACCES
+//   1: success
+//   2: other error
+
+static int try_open(int flags) {
+  int fd = open(file, flags);
+  if (fd >= 0) {
+    close(fd);
+    return 1;
+  }
+  if (errno == EACCES) {
+    return 0;
+  }
+  emscripten_console_error(strerror(errno));
+  return 2;
+}
+
+
+EMSCRIPTEN_KEEPALIVE
+int try_open_wronly(void) {
+  return try_open(O_WRONLY);
+}
+
+EMSCRIPTEN_KEEPALIVE
+int try_open_rdwr(void) {
+  return try_open(O_RDWR);
+}
+
+EMSCRIPTEN_KEEPALIVE
+int try_open_rdonly(void) {
+  return try_open(O_RDONLY);
+}
+
+EMSCRIPTEN_KEEPALIVE
+void report_result(int result) {
+  EM_ASM({ console.log(new Error().stack); });
+#ifdef REPORT_RESULT
+  REPORT_RESULT(result);
+#else
+  if (result != 0) {
+    abort();
+  }
+#endif
+}
diff --git a/test/wasmfs/wasmfs_opfs_errors_post.js b/test/wasmfs/wasmfs_opfs_errors_post.js
@@ -0,0 +1,45 @@
+async function run_test() {
+  let access;
+  try {
+    let root = await navigator.storage.getDirectory();
+    let child = await root.getFileHandle("data", {create: true});
+    access = await child.createSyncAccessHandle();
+  } catch (e) {
+    console.log("test setup failed");
+    throw e;
+  }
+
+  // We won't be able to open the file for writing since there is already a
+  // file handle for it.
+
+  if (Module._try_open_wronly() != 0) {
+    throw "Did not get expected EACCES opening file for writing";
+  }
+
+  if (Module._try_open_rdwr() != 0) {
+    throw "Did not get expected EACCES opening file for reading and writing";
+  }
+
+  if (Module._try_open_rdonly() != 1) {
+    throw "Unexpected failure opening file for reading only";
+  }
+
+  await access.close();
+
+  // We can open the file in any mode now that there is no open access
+  // handle for it.
+
+  if (Module._try_open_wronly() != 1) {
+    throw "Unexpected failure opening file for writing";
+  }
+
+  if (Module._try_open_rdwr() != 1) {
+    throw "Unexpected failure opening file for reading and writing";
+  }
+
+  if (Module._try_open_rdonly() != 1) {
+    throw "Unexpected failure opening file for reading";
+  }
+
+  Module._report_result(0);
+}

Original file line number	Diff line number	Diff line change
`@@ -156,10 +156,7 @@ class NodeFile : public DataFile {`
`156`	`156`	`WASMFS_UNREACHABLE("TODO: implement NodeFile::setSize");`
`157`	`157`	`}`
`158`	`158`
`159`		`- void open(oflags_t flags) override {`
`160`		`- // TODO: Properly report errors.`
`161`		`- state.open(flags);`
`162`		`- }`
	`159`	`+ int open(oflags_t flags) override { return state.open(flags); }`
`163`	`160`
`164`	`161`	`void close() override { state.close(); }`
`165`	`162`
Original file line number	Diff line number	Diff line change
`@@ -272,12 +272,7 @@ class OPFSFile : public DataFile {`
`272`	`272`	`}`
`273`	`273`	`}`
`274`	`274`
`275`		`- void open(oflags_t flags) override {`
`276`		`- if (auto err = state.open(proxy, fileID, flags); err < 0) {`
`277`		`- // TODO: Proper error reporting.`
`278`		`- assert(false && "error during open");`
`279`		`- }`
`280`		`- }`
	`275`	`+ int open(oflags_t flags) override { return state.open(proxy, fileID, flags); }`
`281`	`276`
`282`	`277`	`void close() override { state.close(proxy); }`
`283`	`278`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ class JSImplFile : public DataFile {`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`// TODO: Notify the JS about open and close events?`
`80`		`- void open(oflags_t) override {}`
	`80`	`+ int open(oflags_t) override { return 0; }`
`81`	`81`	`void close() override {}`
`82`	`82`
`83`	`83`	`ssize_t write(const uint8_t* buf, size_t len, off_t offset) override {`