Merge pull request #820 from squeek502/ci-gc-lua-5.5

ci: Update aggressive GC settings for Lua 5.5, fix potential use-after-frees

Author: squeek502

.github/workflows/ci.yml

@@ -49,7 +49,9 @@ jobs:
       run: make test
     - name: Test with Aggressive GC
       # Run with extremely aggressive garbage collection to potentially find more problems
-      run: ./build/lua -e "collectgarbage('incremental', 0, 10000000000000)" tests/run.lua
+      # The 0 for stepmul is "a special case, [...] effectively producing a non-incremental, stop-the-world collector."
+      # These calls assume Lua 5.5. Earlier versions had different APIs for setting the equivalent of these settings.
+      run: ./build/lua -e "collectgarbage('incremental'); collectgarbage('param', 'pause', 0); collectgarbage('param', 'stepmul', 0);" tests/run.lua
 
   clang-tsan:
     runs-on: ubuntu-latest
@@ -68,7 +70,9 @@ jobs:
       run: make test
     - name: Test with Aggressive GC
       # Run with extremely aggressive garbage collection to potentially find more problems
-      run: ./build/lua -e "collectgarbage('incremental', 0, 10000000000000)" tests/run.lua
+      # The 0 for stepmul is "a special case, [...] effectively producing a non-incremental, stop-the-world collector."
+      # These calls assume Lua 5.5. Earlier versions had different APIs for setting the equivalent of these settings.
+      run: ./build/lua -e "collectgarbage('incremental'); collectgarbage('param', 'pause', 0); collectgarbage('param', 'stepmul', 0);" tests/run.lua
 
   valgrind:
     runs-on: ubuntu-latest
@@ -86,7 +90,7 @@ jobs:
     - name: Build
       run: make
     - name: Test
-      run: valgrind --suppressions=.ci/valgrind_mem.supp --error-exitcode=1 --leak-check=full --child-silent-after-fork=yes --keep-debuginfo=yes --track-origins=yes ./build/lua -e "collectgarbage('incremental', 0, 10000000000000)" tests/run.lua
+      run: valgrind --suppressions=.ci/valgrind_mem.supp --error-exitcode=1 --leak-check=full --child-silent-after-fork=yes --keep-debuginfo=yes --track-origins=yes ./build/lua -e "collectgarbage('incremental'); collectgarbage('param', 'pause', 0); collectgarbage('param', 'stepmul', 0);" tests/run.lua
 
   process-cleanup-test:
     runs-on: ubuntu-latest

src/fs.c

@@ -260,13 +260,6 @@ static int push_fs_result(lua_State* L, uv_fs_t* req) {
   }
 
   if (req->result < 0) {
-    if (req->fs_type == UV_FS_SCANDIR) {
-      // We need to unref the luv_fs_scandir_t userdata to allow it to be garbage collected.
-      // The scandir callback can only be called once, so we now know that the
-      // req can be safely garbage collected.
-      luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
-      data->data_ref = LUA_NOREF;
-    }
     lua_pushnil(L);
     if (fs_req_has_dest_path(req)) {
       lua_rawgeti(L, LUA_REGISTRYINDEX, data->data_ref);
@@ -356,11 +349,6 @@ static int push_fs_result(lua_State* L, uv_fs_t* req) {
       // We want to return this instead of the uv_req_t because the
       // luv_fs_scandir_t userdata has a gc method.
       lua_rawgeti(L, LUA_REGISTRYINDEX, data->data_ref);
-      // We now want to unref the userdata to allow it to be garbage collected.
-      // The scandir callback can only be called once, so we now know that the
-      // req can be safely garbage collected.
-      luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
-      data->data_ref = LUA_NOREF;
       return 1;
 
 #if LUV_UV_VERSION_GEQ(1, 28, 0)
@@ -386,9 +374,6 @@ static int push_fs_result(lua_State* L, uv_fs_t* req) {
       return 1;
     }
     case UV_FS_READDIR: {
-      luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
-      data->data_ref = LUA_NOREF;
-
       if(req->result > 0) {
         size_t i;
         uv_dir_t *dir = (uv_dir_t*)req->ptr;
@@ -400,6 +385,8 @@ static int push_fs_result(lua_State* L, uv_fs_t* req) {
       } else
         lua_pushnil(L);
 
+      luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
+      data->data_ref = LUA_NOREF;
       return 1;
     }
     case UV_FS_CLOSEDIR:
@@ -438,6 +425,15 @@ static void luv_fs_cb(uv_fs_t* req) {
   }
   if (req->fs_type == UV_FS_SCANDIR) {
     luv_fulfill_req(L, data, nargs);
+    // Regardless of whether or not we errored, we need to unref the
+    // luv_fs_scandir_t userdata to allow it to be garbage collected.
+    // The scandir callback can only be called once, so we now know that
+    // the req can be safely garbage collected.
+    //
+    // Crucially, this needs to happen after the fulfill_req call to ensure
+    // it doesn't get garbage collected beforehand.
+    luaL_unref(L, LUA_REGISTRYINDEX, data->data_ref);
+    data->data_ref = LUA_NOREF;
   }
   else {
     // cleanup the uv_fs_t before the callback is called to avoid

tests/test-fs.lua

@@ -82,9 +82,6 @@ return require('lib/tap')(function (test)
         assert(uv.fs_unlink(path))
       end)
     end
-    local count = collectgarbage("count")
-    collectgarbage("collect")
-    assert(count - collectgarbage("count") > 0)
   end)
 
   test("fs.stat sync", function (print, p, expect, uv)