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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Payo Nel
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.

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"))

The intention here is simply to load a string with more than 40 chars, to get outside the short string optimization and cause loadStringN to allocate a TString, and then try to write directly to its buffer. The collectgarbage method is called inside the load reader, causing the not-in-stack TString to be free'd. If using a test build, the free'd memory will get AB values written to its memory address first, easily hitting an abort shortly there after

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.



Reply | Threaded
Open this post in threaded view
|

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

Roberto Ierusalimschy
> 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
Reply | Threaded
Open this post in threaded view
|

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

Roberto Ierusalimschy
> 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 ()
>   [...]
> ---------------------------

I think this function is already implemented in testes/call.lua under
the name 'read1'.

-- Roberto
Reply | Threaded
Open this post in threaded view
|

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

Petite Abeille
In reply to this post by Roberto Ierusalimschy


> On Aug 17, 2020, at 18:21, Roberto Ierusalimschy <[hidden email]> wrote:
>
> You right! Thanks for the report.

Perhaps of interest:

The Relevance of Classic Fuzz Testing: Have We Solved This One?
https://ftp.cs.wisc.edu/paradyn/technical_papers/fuzz2020.pdf