From 02d703b42be44d31483582a6538e885fa9b37134 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Tue, 31 May 2022 01:27:43 -0700 Subject: [PATCH] Fix leak from `fs_scandir` whenever it wasn't fully iterated via `fs_scandir_next` (#601) 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 errored out in `luv_check_fs`). Closes #600 --- src/fs.c | 20 +++++++++++++------- src/lreq.c | 8 ++++++-- src/luv.c | 11 +++++++++++ src/private.h | 1 + src/req.c | 5 ++++- tests/test-fs.lua | 8 ++++++++ 6 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/fs.c b/src/fs.c index c7c69923..c0cf758e 100644 --- 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,7 +600,7 @@ 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); } @@ -597,12 +608,7 @@ 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 index 062c4fbf..2322dd0e 100644 --- a/src/lreq.c +++ b/src/lreq.c @@ -26,7 +26,7 @@ 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); @@ -34,7 +34,7 @@ static luv_req_t* luv_setup_req(lua_State* L, luv_ctx_t* ctx, int cb_ref) { 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 index 6264262e..f708926d 100644 --- 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 index 9224ccab..1e921a4a 100644 --- 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 index 77b86dea..affc8949 100644 --- 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 index 23942e21..e199b8e5 100644 --- 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)