Skip to content

Commit

Permalink
Fix leak from fs_scandir whenever it wasn't fully iterated via `fs_…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
squeek502 authored May 31, 2022
1 parent 26401e1 commit 02d703b
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 10 deletions.
20 changes: 13 additions & 7 deletions src/fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
8 changes: 6 additions & 2 deletions src/lreq.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
11 changes: 11 additions & 0 deletions src/luv.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/private.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
5 changes: 4 additions & 1 deletion src/req.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions tests/test-fs.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 02d703b

Please sign in to comment.