opendir and friends: Fix race condition when uv_dir_t pointer doesn't…

… change between allocations (#598) If opendir/closedir was called in a loop, it was possible for the uv_dir_t pointer to always be the same between calls, and due to how we kept track of the 'closed' state of the uv_dir_t by its pointer, this could lead to freeing things twice, or freeing the memory before a readdir call, etc. Instead, we now use a wrapping luv_dir_t and normal Lua references to keep track of the state Fixes #597
luvit · May 5, 2022 · c51e705 · c51e705
1 parent d2e2355
commit c51e705
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 28 deletions.
diff --git a/docs.md b/docs.md
@@ -2975,14 +2975,14 @@ Copies a file from path to new_path. If the `flags` parameter is omitted, then t
 - `path`: `string`
 - `callback`: `callable` (async version) or `nil` (sync version)
   - `err`: `nil` or `string`
-  - `dir`: `uv_dir_t userdata` or `nil`
+  - `dir`: `luv_dir_t userdata` or `nil`
 - `entries`: `integer` or `nil`
 
 Opens path as a directory stream. Returns a handle that the user can pass to
 `uv.fs_readdir()`. The `entries` parameter defines the maximum number of entries
 that should be returned by each call to `uv.fs_readdir()`.
 
-**Returns (sync version):** `uv_dir_t userdata` or `fail`
+**Returns (sync version):** `luv_dir_t userdata` or `fail`
 
 **Returns (async version):** `uv_fs_t userdata`
 
@@ -2991,12 +2991,12 @@ that should be returned by each call to `uv.fs_readdir()`.
 > method form `dir:readdir([callback])`
 
 **Parameters:**
-- `dir`: `uv_dir_t userdata`
+- `dir`: `luv_dir_t userdata`
 - `callback`: `callable` (async version) or `nil` (sync version)
   - `err`: `nil` or `string`
   - `entries`: `table` or `nil` (see below)
 
-Iterates over the directory stream `uv_dir_t` returned by a successful
+Iterates over the directory stream `luv_dir_t` returned by a successful
 `uv.fs_opendir()` call. A table of data tables is returned where the number
 of entries `n` is equal to or less than the `entries` parameter used in
 the associated `uv.fs_opendir()` call.
@@ -3013,7 +3013,7 @@ the associated `uv.fs_opendir()` call.
 > method form `dir:closedir([callback])`
 
 **Parameters:**
-- `dir`: `uv_dir_t userdata`
+- `dir`: `luv_dir_t userdata`
 - `callback`: `callable` (async version) or `nil` (sync version)
   - `err`: `nil` or `string`
   - `success`: `boolean` or `nil`

diff --git a/src/fs.c b/src/fs.c
@@ -17,6 +17,13 @@
 
 #include "private.h"
 
+#if LUV_UV_VERSION_GEQ(1, 28, 0)
+typedef struct {
+  uv_dir_t* handle;
+  int dirents_ref; /* handle has been closed if this is LUA_NOREF */
+} luv_dir_t;
+#endif
+
 static uv_fs_t* luv_check_fs(lua_State* L, int index) {
   uv_fs_t* req = (uv_fs_t*)luaL_checkudata(L, index, "uv_req");
   luaL_argcheck(L, req->type == UV_FS && req->data, index, "Expected uv_fs_t");
@@ -333,14 +340,15 @@ static int push_fs_result(lua_State* L, uv_fs_t* req) {
       luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
       data->data_ref = LUA_NOREF;
 
-      (*(void**)lua_newuserdata(L, sizeof(void*))) = dir;
-      lua_pushfstring(L, "uv_dir:%p", dir);
-      dir->dirents = lua_newuserdata(L, sizeof(uv_dirent_t)*nentries);
-      dir->nentries = nentries;
-      lua_rawset(L, LUA_REGISTRYINDEX);
+      luv_dir_t* luv_dir = lua_newuserdata(L, sizeof(*luv_dir));
       luaL_getmetatable(L, "uv_dir");
       lua_setmetatable(L, -2);
 
+      luv_dir->handle = dir;
+      luv_dir->handle->dirents = lua_newuserdata(L, sizeof(uv_dirent_t)*nentries);
+      luv_dir->dirents_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+      luv_dir->handle->nentries = nentries;
+
       return 1;
     }
     case UV_FS_READDIR: {
@@ -888,8 +896,8 @@ static int luv_fs_copyfile(lua_State*L) {
 #endif
 
 #if LUV_UV_VERSION_GEQ(1, 28, 0)
-static uv_dir_t* luv_check_dir(lua_State* L, int idx) {
-  uv_dir_t* dir = *(uv_dir_t**)luaL_checkudata(L, idx, "uv_dir");
+static luv_dir_t* luv_check_dir(lua_State* L, int idx) {
+  luv_dir_t* dir = (luv_dir_t*)luaL_checkudata(L, idx, "uv_dir");
   return dir;
 }
 
@@ -911,45 +919,44 @@ static int luv_fs_opendir(lua_State* L) {
 static int luv_fs_readdir(lua_State* L) {
   luv_ctx_t* ctx = luv_context(L);
   uv_fs_t *req;
-  uv_dir_t* dir = luv_check_dir(L, 1);
+  luv_dir_t* dir = luv_check_dir(L, 1);
   int ref = luv_check_continuation(L, 2);
 
   req = (uv_fs_t*)lua_newuserdata(L, uv_req_size(UV_FS));
   req->data = luv_setup_req(L, ctx, ref);
-  FS_CALL(readdir, req, dir);
+  FS_CALL(readdir, req, dir->handle);
 }
 
 static int luv_fs_closedir(lua_State* L) {
   luv_ctx_t* ctx = luv_context(L);
-  uv_dir_t* dir = luv_check_dir(L, 1);
+  luv_dir_t* dir = luv_check_dir(L, 1);
   int ref = luv_check_continuation(L, 2);
+
+  luaL_unref(L, LUA_REGISTRYINDEX, dir->dirents_ref);
+  dir->dirents_ref = LUA_NOREF;
+
   uv_fs_t *req = (uv_fs_t*)lua_newuserdata(L, uv_req_size(UV_FS));
   req->data = luv_setup_req(L, ctx, ref);
-  lua_pushfstring(L, "uv_dir:%p", dir);
-  lua_pushnil(L);
-  lua_rawset(L, LUA_REGISTRYINDEX);
-  FS_CALL(closedir, req, dir);
+  FS_CALL(closedir, req, dir->handle);
 }
 
 static int luv_fs_dir_tostring(lua_State* L) {
-  uv_dir_t* dir = luv_check_dir(L, 1);
+  luv_dir_t* dir = luv_check_dir(L, 1);
   lua_pushfstring(L, "uv_dir_t: %p", dir);
   return 1;
 }
 
 static int luv_fs_dir_gc(lua_State* L) {
-  uv_dir_t* dir = luv_check_dir(L, 1);
-  lua_pushfstring(L, "uv_dir:%p", dir);
-  lua_rawget(L, LUA_REGISTRYINDEX);
-  if (!lua_isnil(L, -1)) {
+  luv_dir_t* dir = luv_check_dir(L, 1);
+  if (dir->dirents_ref != LUA_NOREF) {
     uv_fs_t req;
     luv_ctx_t* ctx = luv_context(L);
 
-    uv_fs_closedir(ctx->loop, &req, dir, NULL);
+    luaL_unref(L, LUA_REGISTRYINDEX, dir->dirents_ref);
+    dir->dirents_ref = LUA_NOREF;
+
+    uv_fs_closedir(ctx->loop, &req, dir->handle, NULL);
     uv_fs_req_cleanup(&req);
-    lua_pushfstring(L, "uv_dir:%p", dir);
-    lua_pushnil(L);
-    lua_rawset(L, LUA_REGISTRYINDEX);
   }
   lua_pop(L, 1);
 

diff --git a/tests/test-fs.lua b/tests/test-fs.lua
@@ -212,6 +212,17 @@ return require('lib/tap')(function (test)
     assert(uv.fs_opendir('.', opendir_cb, 50))
   end, "1.28.0")
 
+  test("fs.opendir and fs.closedir in a loop", function(print, p, expect, uv)
+    -- Previously, this triggered a GC/closedir race condition
+    -- see https://github.com/luvit/luv/issues/597
+    for _ = 1,1000 do
+      local dir, err = uv.fs_opendir('.', nil, 64)
+      if not err then
+        uv.fs_closedir(dir)
+      end
+    end
+  end, "1.28.0")
+
   test("fs.statfs sync", function (print, p, expect, uv)
     local stat = assert(uv.fs_statfs("."))
     p(stat)