LuaSandbox memory limits should limit the amount of memory actually in use.

@Surjection proposes running a full GC from the allocation hook as a last resort prior to reporting an out-of-memory error.

In T165935#7269782, @Surjection wrote:

LuaSandbox implements its own wrapper for the memory allocator that is passed to Lua explicitly for keeping track of the memory limit (you've allocated 64 bytes after already allocating 50 MB - 32? no allocation for you). Right now, it's clear that LuaSandbox holds to an invariant that there is only lua_State per php_luasandbox_alloc. Because of that, you could just solve it like this:

1. Add two int flags: enable_gc and in_gc to struct php_luasandbox_alloc. Initialize the first one to 1 when creating a new struct php_luasandbox_alloc and the latter to 0.
2. Add a lua_State * field (let's call it state) to struct php_luasandbox_alloc. Initialize this in luasandbox_alloc_new_state and set it to NULL in luasandbox_alloc_delete_state.
   1. If it isn't already NULL in luasandbox_alloc_new_state, that means you have multiple Lua states per alloc. In this case, set enable_gc to 0.
3. In luasandbox_update_memory_accounting: if we exceed the memory limit. check enable_gc and in_gc. if the first one is 1, the second one is 0 and state != NULL, do
   1. set alloc->in_gc to 1
   2. call lua_gc(alloc->state, LUA_GCCOLLECT, 0)
   3. if allocation now succeeds, set alloc->in_gc to 0. if it fails, do that anyway. the check whether we hit the memory limit should happen BEFORE in_gc is set to 0, else we will call GC in an infinite loop.

Calling lua_gc from a lua_Alloc is completely safe given you don't end up calling it again while it's already running (hence the in_gc). GC shouldn't allocate any memory but only free it, so the GC should never fail (but even if it does, all that happens is that the GC fails and then we can return NULL from lua_Alloc safely).

Note that it's only safe to run the GC from the allocation hook if the reference tree is in a well-defined state at that time, i.e. contains no dangling pointers or the like. After reviewing the code for a while, I'm about 80% convinced. It would be safer if we zero-filled newly-allocated memory.

An example of the kind of thing I'm looking for:

void luaS_resize (lua_State *L, int newsize) {
  ...
  if (G(L)->gcstate == GCSsweepstring)
    return;  /* cannot resize during GC traverse */

The converse is also true — you can't GC during a resize of the global string table. But it doesn't allocate memory during the critical section.

We don't allow users to catch a memory allocation failure with pcall(), although that would be possible in base Lua. So there is a motive for upstream to ensure that the global state is consistent when allocation is done.

LUA_API lua_State *lua_newstate (lua_Alloc f, void *ud) {
  ...
  if (luaD_rawrunprotected(L, f_luaopen, NULL) != 0) {
    /* memory allocation error: free partial state */
    close_state(L);
    L = NULL;
  }

This is a concerning form of protection, and it indicates that it's unsafe for us to do a GC during lua_newstate(). This needs special handling since the custom allocator is active at this point.

There are three other calls to luaD_rawrunprotected(), which all look fine.

The libraries should be fine. The lexer looks fine. A problem would look like:

1. Write garbage to global state or a GC object
2. Allocate memory
3. Fix the garbage

Maybe we have that in lua_newstate(). It's hard to be sure since an actual crash would depend on one of about 50 properties not being initialised in the lua_State. Zero-filling would make it much harder to turn such an issue into code execution.