Re: Bug report : A weak table reuse a string key that already free

[彭 书呆 <nerditation@...>]

在 2017/8/16 0:37, 云风 Cloud Wu 写道:
> I found a bug of lua 5.3.4 in our project, but it's hardly to make a simple pure lua minimal reproducible example.
>  so I use a C program to explain it.
> 
> ```C
> #include <lua.h>
> #include <lauxlib.h>
> #include <lualib.h>
> #include <stdlib.h>
> #include <lstring.h>
> 
> static void *
> l_alloc (void *ud, void *ptr, size_t osize, size_t nsize) {
> if (nsize == 0) {
> printf("free %p\n", ptr);
> free(ptr);
> return NULL;
> } else {
> return realloc(ptr, nsize);
> }
> }
> 
> static int
> lpointer(lua_State *L) {
> const char * str = luaL_checkstring(L, 1);
> const TString *ts = (const TString *)str - 1;
> lua_pushlightuserdata(L, (void *)ts);
> return 1;
> }
> 
> const char *source = "\n\
> local a = setmetatable({} , { __mode = 'kv' })\n\
> a['ABCDEFGHIJKLMNOPQRSTUVWXYZ' .. 'abcdefghijklmnopqrstuvwxyz' ] = {}\n\
> print(pointer((next(a))))\n\
> a[next(a)] = nil\n\
> collectgarbage 'collect'\n\
> local key = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ' .. 'abcdefghijklmnopqrstuvwxyz'\n\
> print(pointer(key))\n\
> a[key] = {}\n\
> print(pointer((next(a))))\n\
> ";
> 
> int main() {
> lua_State *L = lua_newstate (l_alloc, NULL);
> luaL_openlibs(L);
> lua_pushcfunction(L, lpointer);
> lua_setglobal(L, "pointer");
> luaL_dostring(L, source);
> 
> return 0;
> }
> ```
> 
> ...
> userdata: 00000000006fedd0     <-   TString 1: ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz
> free 00000000006FAB50     <- GC
> free 00000000006FA890
> free 00000000006FE940
> free 00000000006FE910
> free 0000000000000000
> free 00000000006FA650
> free 00000000006FEDD0      <-  Tstring 1 is free
> free 00000000006FEBC0
> free 0000000000000000
> free 00000000006FAA50
> free 00000000006F9770
> userdata: 00000000006f1eb0   <-   TString 2: ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz
> userdata: 00000000006fedd0   <-   TString 3 is the TString 1, and it has already free.
> 
> -----
> 
> The string "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" is collected by collectgarbage , but the TString * remains in the table 'a' because the value is nil. When we set a new key with the same string , luaH_set use the old slot with TString has already free.
> 
> This patch can fix this bug, but I think there is a better way.
> 
> diff --git a/src/lgc.c b/src/lgc.c
> index ba2c19e..2d06ac5 100644
> --- a/src/lgc.c
> +++ b/src/lgc.c
> @@ -641,7 +641,7 @@ static void clearkeys (global_State *g, GCObject *l, GCObjec     Table *h = gco2t(l);
>      Node *n, *limit = gnodelast(h);
>      for (n = gnode(h, 0); n < limit; n++) {
> -      if (!ttisnil(gval(n)) && (iscleared(g, gkey(n)))) {
> +      if (iscleared(g, gkey(n))) {
>          setnilvalue(gval(n));  /* remove value ... */
>          removeentry(n);  /* and remove entry from table */
>        }
> 

I think this issue only applies to long string keys, right?

one reason long strings are special is that long string is the only type in Lua of which different
objects might be regarded as being equal -- by different objects I mean objects with different allocated
addresses -- as the function ``luaV_equalobj`` shows. this break the assumption that when a key is dead,
there is no way you can later insert a slot with the same key.

I think your fix is correct, although clear dead keys eagerly might cause the table to be unstable, which
might impact the performance of the program in (some???) situations.

another possible fix I could think out is to use the new key object when setting a new slot, either
unconditionally, or just for long strings. but this means the ``luaH_set`` can no longer re-use the code
of ``luaH_get``, e.g. we can move the code of original ``luaH_get`` to a new function ``luaH_getnode``,
something like this (concept, not actual code):


--------------------------------------------------------------------------
/* main search function, returns a dummy node if key is absent */
Node *luaH_getnode (Table *t, const TValue *key);

/* forward to new search function */
const TValue *luaH_get (Table *t, const TValue *key) {
  return gval(luaH_getnode(t, key));
}

/* overwrite old key (which might be dead long string) with new key */
TValue *luaH_set (lua_State *L, Table *t, const TValue *key) {
  const Node *node = luaH_getnode(t, key);
  /* what's a better way than a dummy node??? */
  if (node != &dummynode) {
    if (ttype(gval(node)) == LUA_TLNGSTR) setnodekey(L, wgkey(node), key);
    return gval(node);
  }
  else return luaH_newkey(L, t, key);
}
--------------------------------------------------------------------------

there should be a better way to fix this. but I can only get this.

-- 
the nerdy Peng / 书呆彭 /