SIGILL when using interrupts

[khvzak]

Hello,
Initially discovered in mlua-rs/mlua#142

The following program:

#include <assert.h>
#include <lua.h>
#include <luacode.h>
#include <lualib.h>
#include <iostream>

void interrupt(lua_State* L, int gc) {
  if (gc >= 0) {
    return;
  }
  luaL_checkstack(L, 1, "not enough stack");
}

int main() {
  lua_State* L = luaL_newstate();
  luaL_openlibs(L);

  std::string code = R"END(
        local i = 0
        local function foo()
            i = i + 1
            return i < 1000000
        end
        while foo() do end
        print("done")
    )END";
  size_t data_size = 0;
  auto data = luau_compile(code.c_str(), code.length(), NULL, &data_size);
  assert(luau_load(L, "test", data, data_size, 0) == 0);

  lua_callbacks(L)->interrupt = interrupt;

  if (lua_pcall(L, 0, 0, 0) != LUA_OK) {
    auto err = lua_tostring(L, -1);
    std::cout << err << std::endl;
  }

  lua_close(L);
  return 0;
}

Generates SIGILL (illegal hardware instruction):

$ g++ -I VM/include -I Compiler/include -I Ast/include --std c++17 -O3 VM/src/*.cpp Compiler/src/*.cpp Ast/src/*.cpp test.cpp -o test && ./test
zsh: illegal hardware instruction  ./test

Apple clang version 13.1.6 (clang-1316.0.21.2)
Target: x86_64-apple-darwin21.4.0

Obviously the issue related to inability to allocate more stack, but I'm not sure is this right behaviour?

[zeux]

I am not sure it's safe to change the stack from interrupts in any way; we haven't fully defined the semantics there, but interrupts are not the same as normal C functions. This is sort of intentional because interrupt facility is expected to be extremely performant, so doing any sort of reservation/preparation is not great.

The code above is likely tripping an assert (they are enabled unless you specify -DNDEBUG). You can register the assertion handler like so (no C-only interface atm...):

Luau::assertHandler() = handler;

Once you do, you should get an actual assertion text assuming handler prints it to output.

[khvzak]

I added assertion handler, it prints:

VM/src/lapi.cpp(585): ASSERTION FAILED: L->top < L->ci->top

Does it means that I can only (safely) inspect stack and yield?

[khvzak]

Also I see the following code in fuzz/proto.cpp#L50:

void interrupt(lua_State* L, int gc)
{
    if (gc >= 0)
        return;

    if (std::chrono::system_clock::now() > interruptDeadline)
    {
        lua_checkstack(L, 1);
        luaL_error(L, "execution timed out");
    }
}

if I right, the code has UB (undefined behaviour)?

[zeux]

All great questions :) We'll look into this... To emit an error you need to reserve stack space so it's not fully clear to me why checkstack doesn't work. As I noted we don't really have a documented set of APIs that are safe to use from interrupts but it looks like we need one.

[zeux]

Ahh I see, this is fascinating. The assertion happens during handling of Luau stack overflow. That seems to be a bug.

The reason why Luau stack overflow happens in the first place is the odd interaction between how interpreter loop handles stack frame restore, and checkstack.

So using checkstack followed by an error is safe. Using checkstack that's not followed by an error is not safe because the stack seems to grow on every iteration.

Do you need to reserve stack space in case the error is not emitted? If you don't that's an easy workaround. I'll need to look into what we can fix in the VM here in either case.

[khvzak]

Thanks for the explanation!

In Rust in mlua I have a special wrapper function that does some checks before every function call. The wrapper is also used to capture Rust exceptions and enabled for interrupt handler.

Given that it's not safe to do any arbitrary operations in interrupts (like execting Lua functions / etc) seems would be reasonable to change interface on my side and allow only throwing errors / yielding / continue execution.
In that case I can reserve stack space only for throwing errors.