Re: Multiple Lua Threads, __gc crash accessing "dead" thread

[Daurnimator <quae@...>]

On 14 September 2017 at 00:02, ThePhD <jm3689@columbia.edu> wrote:
> Observe the following complete, compile-able (and mostly C) code, which
> stores a simple "luaL_ref" wrapper in a struct named "co_test" and links up
> a simple creation method ("new") and a simple deletion method ("__gc"):
>
> #include <lua.h>
> #include <lualib.h>
> #include <lauxlib.h>
>
> #include <assert.h>
>
> typedef struct co_test {
>     lua_State* L;
>     int ref;
> } co_test;
>
> void co_test_make(co_test* p, lua_State* L, int stack_target) {
>     lua_pushvalue(L, stack_target); // copy value
>     p->L = L;
>     p->ref = luaL_ref(L, LUA_REGISTRYINDEX); // save in registry
> }
>
> void co_test_destroy(co_test* p) {
>     luaL_unref(p->L, LUA_REGISTRYINDEX, p->ref);
>     p->ref = 0;
>     p->L = NULL;
> }
>
> int co_test_new(lua_State* L) {
>     co_test* p = (co_test*)lua_newuserdata(L, sizeof(co_test));
>     luaL_setmetatable(L, "co_test"); // bless userdata with metatable
>
>     co_test_make(p, L, 1);
>
>     return 1;
> }
>
> int co_test___gc(lua_State* L) {
>     co_test* p = (co_test*)lua_touserdata(L, 1);
>
>     co_test_destroy(p);
>
>     return 0;
> }
>
> int main(int, char*[]) {
>
>     const char code[] = R"(
> local co = coroutine.wrap(
>     function()
>         local t = co_test.new({ "SOME_TABLE" })
>         -- STUFF
>     end
> )
>
> co() -- call
> co = nil -- destroy coroutine/underlying thread
> collectgarbage() -- collect garbage
> )";
>
>     lua_State* L = luaL_newstate();
>     luaL_Reg registration[3] = { 0 };
>     int pre_stack_check = 0;
>     int post_stack_check = 0;
>
>     luaL_requiref(L, "base", luaopen_base, 1);
>     lua_pop(L, 1);
>
>     luaL_requiref(L, "coroutine", luaopen_coroutine, 1);
>     lua_pop(L, 1);
>
>     pre_stack_check = lua_gettop(L);
>     if (luaL_newmetatable(L, "co_test") == 1) {
>         registration[0] = luaL_Reg{ "new", &co_test_new };
>         registration[1] = luaL_Reg{ "__gc", &co_test___gc };
>         luaL_setfuncs(L, registration, 0);
>         lua_pushvalue(L, -1); // duplicate metatable
>         lua_setfield(L, -1, "__index"); // loop-back lookup
>     }
>     lua_setglobal(L, "co_test");
>     post_stack_check = lua_gettop(L);
>     // sanity check
>     assert(pre_stack_check == post_stack_check);
>
>     if (luaL_dostring(L, code)) {
>         assert(false);
>     }
>
>     return 0;
> }
>
>
> In "co_test_destroy", the code segfaults (g++, clang++) / "Read Access
> Violation" (VC++) when doing "luaL_unref" when this code executes. I found
> that the reason is that the "co" variable, which holds the coroutine and
> thread represented by the "L" that the "co_test" was created with, dies. If
> you do not explicitly "nil" the "co" variable in the Lua code, the
> thread/state that "co_test" is created with stays alive.
>
> This behavior is observable in Lua 5.3, 5.2 and 5.1 (but not with LuaJIT on
> VC++ and Windows, for some reason).
>
> I am unsure of how to get around this issue properly. I know in 5.2 and 5.3
> I can forcefully obtain the main thread's state and thusly avoid this
> problem altogether by just adjusting the "L" pointer in the "co_test_new"
> function. But I seem to be on an island for Lua 5.1.
>
> Does anyone have any thoughts about this? Is this behavior desirable? Should
> I just ask my users to collect garbage before setting any coroutine/thread
> wrappers to "nil"?
>
> Sincerely,
> ThePhD

I was able to run your testcase without issue in 5.2 and 5.3. (5.1
failed due to missing functions e.g. luaL_setmetatable)

Patch to make it compile in normal gcc + some debugging output:

--- phd-orig.c    2017-09-14 00:32:14.094458804 +1000
+++ phd.c    2017-09-14 00:33:00.193862671 +1000
@@ -17,4 +17,5 @@

 void co_test_destroy(co_test* p) {
+    printf("COLLECTING\n");
     luaL_unref(p->L, LUA_REGISTRYINDEX, p->ref);
     p->ref = 0;
@@ -39,5 +40,5 @@
 }

-int main(int, char*[]) {
+int main() {

     const char code[] = R"(
@@ -52,8 +53,13 @@
 co = nil -- destroy coroutine/underlying thread
 collectgarbage() -- collect garbage
+print('DONE')
 )";

     lua_State* L = luaL_newstate();
-    luaL_Reg registration[3] = { 0 };
+    luaL_Reg registration[] = {
+        { "new", &co_test_new },
+        { "__gc", &co_test___gc },
+        { NULL, NULL }
+    };
     int pre_stack_check = 0;
     int post_stack_check = 0;
@@ -67,6 +73,4 @@
     pre_stack_check = lua_gettop(L);
     if (luaL_newmetatable(L, "co_test") == 1) {
-        registration[0] = luaL_Reg{ "new", &co_test_new };
-        registration[1] = luaL_Reg{ "__gc", &co_test___gc };
         luaL_setfuncs(L, registration, 0);
         lua_pushvalue(L, -1); // duplicate metatable
@@ -79,5 +83,5 @@

     if (luaL_dostring(L, code)) {
-        assert(false);
+        assert(0);
     }