Re: loadStringN can write to freed memory, will fail assert in tests

[Roberto Ierusalimschy <roberto@...>]

> I noticed loadStringN was diverging between using a local stack array
> for short string loading vs a dynamically allocated TString buffer for
> long strings. And I noticed that a test in calls.lua uses a custom
> string reader function passed to load. The custom read1 method in
> calls.lua also invokes the garabagecollector. Turns out, you could
> cause the TString allocated for long string loads to be freed during
> load.

You right! Thanks for the report.


> to repro
> go to line 320 in calls.lua -> https://github.com/lua/lua/blob/master/testes/calls.lua#L320
> change the line from:
> - x = string.dump(load("x = 1; return x"))
> to
> + x = string.dump(load("x = 1; return                                                                                                                                    x"))

I am afraid that change does not ensure that there is a GC while reading
the long string, so it may not trigger the bug. (It didn't when I tried.)
The following chunk seems more reliable:

---------------------------
-- run this chunck under some memory checker
local function myload (s)
  return load(function ()
    if s == "" then return nil
    else
      local c = string.sub(s, 1, 1)
      s = string.sub(s, 2)
      collectgarbage()
      return c
    end
  end)
end


y = string.dump(function ()
  return '01234567890123456789012345678901234567890123456789'
end)
y = myload(y)
assert(y() == '01234567890123456789012345678901234567890123456789')
---------------------------


> I'm not sure what the best solution to this would be, my fix is to
> push the TString on the stack to protect it from gc.
> 
> static TString *loadStringN (LoadState *S, Proto *p) {
>    ....
>     setsvalue2s(L, L->top, ts);  /* push ts on stack to protect from possible gc */
>     api_incr_top(L);
>     loadVector(S, getstr(ts), size);  /* load directly in final place */
>     lua_unlock(L);
>     lua_settop(L, -2);
>     lua_lock(L);
> 
> I don't think my solution is very good, I am interested in what the
> community proposes to fix this.

The standard solution is to anchor the string in the stack, as you
proposed. However, the code doesn't need to/should call lua_settop. It
can simply decrement top.

-- Roberto