Fix leak from fs_scandir whenever it wasn't fully iterated via `fs_…

…scandir_next` Now, we have a specialized metatable ("uv_fs") with a gc function that's only used for scandir's uv_fs_t reqs, so that it will always be cleaned up automatically. This also means that once fully iterated, the req is no longer immediately cleaned up and so any subsequent calls to `fs_scandir_next` will just return `nil` (whereas previously it would have invoked use-after-free).
luvit · May 31, 2022 · 12c2625 · 12c2625
1 parent c51e705
commit 12c2625
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 10 deletions.
diff --git a/src/fs.c b/src/fs.c
@@ -25,11 +25,22 @@ typedef struct {
 #endif
 
 static uv_fs_t* luv_check_fs(lua_State* L, int index) {
+  if (luaL_testudata(L, index, "uv_fs") != NULL) {
+    return (uv_fs_t*)lua_touserdata(L, 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");
   return req;
 }
 
+static int luv_fs_gc(lua_State* L) {
+  uv_fs_t* req = luv_check_fs(L, 1);
+  luv_cleanup_req(L, (luv_req_t*)req->data);
+  req->data = NULL;
+  uv_fs_req_cleanup(req);
+  return 0;
+}
+
 static void luv_push_timespec_table(lua_State* L, const uv_timespec_t* t) {
   lua_createtable(L, 0, 2);
   lua_pushinteger(L, t->tv_sec);
@@ -589,20 +600,15 @@ static int luv_fs_scandir(lua_State* L) {
   int flags = 0; // TODO: find out what these flags are.
   int ref = luv_check_continuation(L, 2);
   uv_fs_t* req = (uv_fs_t*)lua_newuserdata(L, uv_req_size(UV_FS));
-  req->data = luv_setup_req(L, ctx, ref);
+  req->data = luv_setup_req_with_mt(L, ctx, ref, "uv_fs");
   FS_CALL(scandir, req, path, flags);
 }
 
 static int luv_fs_scandir_next(lua_State* L) {
   uv_fs_t* req = luv_check_fs(L, 1);
   uv_dirent_t ent;
   int ret = uv_fs_scandir_next(req, &ent);
-  if (ret == UV_EOF) {
-    luv_cleanup_req(L, (luv_req_t*)req->data);
-    req->data = NULL;
-    uv_fs_req_cleanup(req);
-    return 0;
-  }
+  if (ret == UV_EOF) return 0;
   if (ret < 0) return luv_error(L, ret);
   return luv_push_dirent(L, &ent, 0);
 }

diff --git a/src/lreq.c b/src/lreq.c
@@ -26,15 +26,15 @@ static int luv_check_continuation(lua_State* L, int index) {
 
 // Store a lua callback in a luv_req for the continuation.
 // The uv_req_t is assumed to be at the top of the stack
-static luv_req_t* luv_setup_req(lua_State* L, luv_ctx_t* ctx, int cb_ref) {
+static luv_req_t* luv_setup_req_with_mt(lua_State* L, luv_ctx_t* ctx, int cb_ref, const char* mt_name) {
   luv_req_t* data;
 
   luaL_checktype(L, -1, LUA_TUSERDATA);
 
   data = (luv_req_t*)malloc(sizeof(*data));
   if (!data) luaL_error(L, "Problem allocating luv request");
 
-  luaL_getmetatable(L, "uv_req");
+  luaL_getmetatable(L, mt_name);
   lua_setmetatable(L, -2);
 
   lua_pushvalue(L, -1);
@@ -47,6 +47,10 @@ static luv_req_t* luv_setup_req(lua_State* L, luv_ctx_t* ctx, int cb_ref) {
   return data;
 }
 
+static luv_req_t* luv_setup_req(lua_State* L, luv_ctx_t* ctx, int cb_ref) {
+  return luv_setup_req_with_mt(L, ctx, cb_ref, "uv_req");
+}
+
 
 static void luv_fulfill_req(lua_State* L, luv_req_t* data, int nargs) {
   if (data->callback_ref == LUA_NOREF) {

diff --git a/src/luv.c b/src/luv.c
@@ -648,6 +648,17 @@ static void luv_req_init(lua_State* L) {
   luaL_newlib(L, luv_req_methods);
   lua_setfield(L, -2, "__index");
   lua_pop(L, 1);
+
+  // Only used for things that need to be garbage collected
+  // (e.g. the req when using uv_fs_scandir)
+  luaL_newmetatable(L, "uv_fs");
+  lua_pushcfunction(L, luv_req_tostring);
+  lua_setfield(L, -2, "__tostring");
+  luaL_newlib(L, luv_req_methods);
+  lua_setfield(L, -2, "__index");
+  lua_pushcfunction(L, luv_fs_gc);
+  lua_setfield(L, -2, "__gc");
+  lua_pop(L, 1);
 }
 
 // Call lua function, will pop nargs values from top of vm stack and push some

diff --git a/src/private.h b/src/private.h
@@ -54,6 +54,7 @@ static int luv_check_continuation(lua_State* L, int index);
    top of the stack.
 */
 static luv_req_t* luv_setup_req(lua_State* L, luv_ctx_t* ctx, int ref);
+static luv_req_t* luv_setup_req_with_mt(lua_State* L, luv_ctx_t* ctx, int ref, const char* mt_name);
 static void luv_fulfill_req(lua_State* L, luv_req_t* data, int nargs);
 static void luv_cleanup_req(lua_State* L, luv_req_t* data);
 

diff --git a/src/req.c b/src/req.c
@@ -17,13 +17,16 @@
 #include "private.h"
 
 static uv_req_t* luv_check_req(lua_State* L, int index) {
+  if (luaL_testudata(L, index, "uv_fs") != NULL) {
+    return (uv_req_t*)lua_touserdata(L, index);
+  }
   uv_req_t* req = (uv_req_t*)luaL_checkudata(L, index, "uv_req");
   luaL_argcheck(L, req->data, index, "Expected uv_req_t");
   return req;
 }
 
 static int luv_req_tostring(lua_State* L) {
-  uv_req_t* req = (uv_req_t*)luaL_checkudata(L, 1, "uv_req");
+  uv_req_t* req = luv_check_req(L, 1);
   switch (req->type) {
 #define XX(uc, lc) case UV_##uc: lua_pushfstring(L, "uv_"#lc"_t: %p", req); break;
   UV_REQ_TYPE_MAP(XX)

diff --git a/tests/test-fs.lua b/tests/test-fs.lua
@@ -125,6 +125,14 @@ return require('lib/tap')(function (test)
     end
   end)
 
+  -- this test does nothing on its own, but when run with a leak checker,
+  -- it will check that the memory allocated by Libuv for req is cleaned up
+  -- even if its not iterated fully (or at all)
+  test("fs.scandir with no iteration", function(print, p, expect, uv)
+    local req = uv.fs_scandir('.')
+    assert(req)
+  end)
+
   test("fs.realpath", function (print, p, expect, uv)
     p(assert(uv.fs_realpath('.')))
     assert(uv.fs_realpath('.', expect(function (err, path)