[Tarantool-patches] [PATCH luajit] Fix frame for on-trace out-of-memory error.

[Tarantool-patches] [PATCH luajit] Fix frame for on-trace out-of-memory error.

[Maxim Kokryashkin via Tarantool-patches]

Reported by ruidong007.

(cherry-picked from commit 2d8300c1944f3a62c10f0829e9b7847c5a6f0482)

When an on-trace OOM error is triggered from a frame that is
child in regard to `jit_base`, and `L->base` is not updated
correspondingly (FUNCC, for example), it is possible to
encounter an inconsistent Lua stack in the error handler.

This patch adds a fixup for OOM errors on trace that always
sets the Lua stack base to `jit_base`, so the stack is
now consistent.

Part of tarantool/tarantool#8825
---
PR: https://github.com/tarantool/tarantool/pull/8909
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-oom-error-frame
 src/lj_err.c                                  |  4 ++++
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-1004-oom-error-frame.test.lua          | 24 +++++++++++++++++++
 .../lj-1004-oom-error-frame/CMakeLists.txt    |  1 +
 .../lj-1004-oom-error-frame/testoomframe.c    | 17 +++++++++++++
 5 files changed, 47 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame.test.lua
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c

diff --git a/src/lj_err.c b/src/lj_err.c
index 9903d273..09729791 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -802,6 +802,10 @@ LJ_NOINLINE void lj_err_mem(lua_State *L)
 {
   if (L->status == LUA_ERRERR+1)  /* Don't touch the stack during lua_open. */
     lj_vm_unwind_c(L->cframe, LUA_ERRMEM);
+  if (LJ_HASJIT) {
+    TValue *base = tvref(G(L)->jit_base);
+    if (base) L->base = base;
+  }
   if (curr_funcisL(L)) L->top = curr_topL(L);
   setstrV(L, L->top++, lj_err_str(L, LJ_ERR_ERRMEM));
   lj_err_throw(L, LUA_ERRMEM);
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 6218f76a..93230677 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -66,6 +66,7 @@ add_subdirectory(lj-416-xor-before-jcc)
 add_subdirectory(lj-601-fix-gc-finderrfunc)
 add_subdirectory(lj-727-lightuserdata-itern)
 add_subdirectory(lj-flush-on-trace)
+add_subdirectory(lj-1004-oom-error-frame)
 
 # The part of the memory profiler toolchain is located in tools
 # directory, jit, profiler, and bytecode toolchains are located
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
new file mode 100644
index 00000000..fd167d14
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
@@ -0,0 +1,24 @@
+local tap = require('tap')
+local test  = tap.test('lj-1004-oom-error-frame'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Test requires GC64 mode disabled'] = require('ffi').abi('gc64'),
+})
+
+test:plan(1)
+
+local testoomframe = require('testoomframe')
+
+local anchor = {}
+local function extra_frame(val)
+  table.insert(anchor, val)
+end
+
+local function chomp()
+  while true do
+    extra_frame(testoomframe.allocate_userdata())
+  end
+end
+
+local st, _ = pcall(chomp)
+test:ok(st == false, 'on-trace error handled successfully')
+os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
new file mode 100644
index 00000000..3bca5df8
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(testoomframe testoomframe.c)
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
new file mode 100644
index 00000000..13071b4e
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -0,0 +1,17 @@
+#include <lua.h>
+#include <lauxlib.h>
+
+static int allocate_userdata(lua_State *L) {
+	lua_newuserdata(L, 16);
+	return 1;
+}
+
+static const struct luaL_Reg testoomframe[] = {
+	{"allocate_userdata", allocate_userdata},
+	{NULL, NULL}
+};
+
+LUA_API int luaopen_testoomframe(lua_State *L) {
+	luaL_register(L, "testoomframe", testoomframe);
+	return 1;
+}
-- 
2.41.0

Re: [Tarantool-patches] [PATCH luajit] Fix frame for on-trace out-of-memory error.

[Sergey Bronnikov via Tarantool-patches]

Max,

Thanks for the patch! LGTM


P.S. I've made an attempt to reproduce the original issue without a 
portion of Lua C code

and build userdata with calling "newproxy()", but attempt is failed, so  
seems Lua C is required here.

Also I'm upset that test triggers an OOM but we cannot reliably distinct 
that

triggered OOM is exactly that OOM that we want. Discussed this verbally,

perhaps such places should be closed with other tests


Sergey


On 7/24/23 17:41, Maxim Kokryashkin wrote:
> Reported by ruidong007.
>
> (cherry-picked from commit 2d8300c1944f3a62c10f0829e9b7847c5a6f0482)
>
> When an on-trace OOM error is triggered from a frame that is
> child in regard to `jit_base`, and `L->base` is not updated
> correspondingly (FUNCC, for example), it is possible to
> encounter an inconsistent Lua stack in the error handler.
>
> This patch adds a fixup for OOM errors on trace that always
> sets the Lua stack base to `jit_base`, so the stack is
> now consistent.
>
> Part of tarantool/tarantool#8825
> ---
> PR: https://github.com/tarantool/tarantool/pull/8909
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-oom-error-frame

LJ issue: https://github.com/LuaJIT/LuaJIT/issues/1004


>   src/lj_err.c                                  |  4 ++++
>   test/tarantool-tests/CMakeLists.txt           |  1 +
>   .../lj-1004-oom-error-frame.test.lua          | 24 +++++++++++++++++++
>   .../lj-1004-oom-error-frame/CMakeLists.txt    |  1 +
>   .../lj-1004-oom-error-frame/testoomframe.c    | 17 +++++++++++++
>   5 files changed, 47 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame.test.lua
>   create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
>   create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
>
> diff --git a/src/lj_err.c b/src/lj_err.c
> index 9903d273..09729791 100644
> --- a/src/lj_err.c
> +++ b/src/lj_err.c
> @@ -802,6 +802,10 @@ LJ_NOINLINE void lj_err_mem(lua_State *L)
>   {
>     if (L->status == LUA_ERRERR+1)  /* Don't touch the stack during lua_open. */
>       lj_vm_unwind_c(L->cframe, LUA_ERRMEM);
> +  if (LJ_HASJIT) {
> +    TValue *base = tvref(G(L)->jit_base);
> +    if (base) L->base = base;
> +  }
>     if (curr_funcisL(L)) L->top = curr_topL(L);
>     setstrV(L, L->top++, lj_err_str(L, LJ_ERR_ERRMEM));
>     lj_err_throw(L, LUA_ERRMEM);
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 6218f76a..93230677 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -66,6 +66,7 @@ add_subdirectory(lj-416-xor-before-jcc)
>   add_subdirectory(lj-601-fix-gc-finderrfunc)
>   add_subdirectory(lj-727-lightuserdata-itern)
>   add_subdirectory(lj-flush-on-trace)
> +add_subdirectory(lj-1004-oom-error-frame)
>   
>   # The part of the memory profiler toolchain is located in tools
>   # directory, jit, profiler, and bytecode toolchains are located
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
> new file mode 100644
> index 00000000..fd167d14
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
> @@ -0,0 +1,24 @@
> +local tap = require('tap')
> +local test  = tap.test('lj-1004-oom-error-frame'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Test requires GC64 mode disabled'] = require('ffi').abi('gc64'),
> +})
> +
> +test:plan(1)
> +
> +local testoomframe = require('testoomframe')
> +
> +local anchor = {}
> +local function extra_frame(val)
> +  table.insert(anchor, val)
> +end
> +
> +local function chomp()
> +  while true do
> +    extra_frame(testoomframe.allocate_userdata())
> +  end
> +end
> +
> +local st, _ = pcall(chomp)
> +test:ok(st == false, 'on-trace error handled successfully')
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> new file mode 100644
> index 00000000..3bca5df8
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> @@ -0,0 +1 @@
> +BuildTestCLib(testoomframe testoomframe.c)
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> new file mode 100644
> index 00000000..13071b4e
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> @@ -0,0 +1,17 @@
> +#include <lua.h>
> +#include <lauxlib.h>
> +
> +static int allocate_userdata(lua_State *L) {
> +	lua_newuserdata(L, 16);
> +	return 1;
> +}
> +
> +static const struct luaL_Reg testoomframe[] = {
> +	{"allocate_userdata", allocate_userdata},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_testoomframe(lua_State *L) {
> +	luaL_register(L, "testoomframe", testoomframe);
> +	return 1;
> +}

Re: [Tarantool-patches] [PATCH luajit] Fix frame for on-trace out-of-memory error.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.

On 24.07.23, Maxim Kokryashkin wrote:
> Reported by ruidong007.
> 
> (cherry-picked from commit 2d8300c1944f3a62c10f0829e9b7847c5a6f0482)
> 
> When an on-trace OOM error is triggered from a frame that is
> child in regard to `jit_base`, and `L->base` is not updated
> correspondingly (FUNCC, for example), it is possible to
> encounter an inconsistent Lua stack in the error handler.
> 
> This patch adds a fixup for OOM errors on trace that always

Typo: s/on trace/on the trace/

> sets the Lua stack base to `jit_base`, so the stack is
> now consistent.
> 
> Part of tarantool/tarantool#8825
> ---

<snipped>

> +local testoomframe = require('testoomframe')
> +
> +local anchor = {}
> +local function extra_frame(val)
> +  table.insert(anchor, val)
> +end
> +
> +local function chomp()
> +  while true do
> +    extra_frame(testoomframe.allocate_userdata())
> +  end
> +end


Before the patch the test takes really a lot of time to wait:
| time LUA_PATH="src/?.lua;test/tarantool-tests/?.lua;;" LUA_CPATH="test/tarantool-tests/lj-1004-oom-error-frame/?.so;;" src/luajit test/tarantool-tests/lj-1
| 004-oom-error-frame.test.lua 
| TAP version 13
| 1..1
| ok - on-trace error handled successfully
|
| real    0m12.984s
| user    0m12.207s
| sys     0m0.775s


I've added the simple chunk eater (and reduce lightuserdata size, just
in case). And this speedups test x2. But I believe that we can make it
even x20 faster with careful size calculations. Also, I suggest to try
to use some non-compilable fast function instead Lua C call to forcify
trace stitching (looks like the error in userdata allocations is the
main blocker).

===================================================================
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
index fd167d14..f8dde4e6 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
@@ -1,13 +1,25 @@
 local tap = require('tap')
+local ffi = require('ffi')
 local test  = tap.test('lj-1004-oom-error-frame'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
-  ['Test requires GC64 mode disabled'] = require('ffi').abi('gc64'),
+  ['Test requires GC64 mode disabled'] = ffi.abi('gc64'),
 })
 
 test:plan(1)
 
 local testoomframe = require('testoomframe')
 
+local anchor_memory = {} -- luacheck: no unused
+local function eatchunks(size)
+  while true do
+    anchor_memory[ffi.new('char[?]', size)] = 1
+  end
+end
+
+if not ffi.abi('gc64') then
+  local r,e = pcall(eatchunks, 512 * 1024 * 1024)
+end
+
 local anchor = {}
 local function extra_frame(val)
   table.insert(anchor, val)
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
index 13071b4e..a54eac63 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -2,7 +2,7 @@
 #include <lauxlib.h>
 
 static int allocate_userdata(lua_State *L) {
-        lua_newuserdata(L, 16);
+        lua_newuserdata(L, 1);
         return 1;
 }
 
===================================================================

| time LUA_PATH="src/?.lua;test/tarantool-tests/?.lua;;" LUA_CPATH="test/tarantool-tests/lj-1004-oom-error-frame/?.so;;" src/luajit test/tarantool-tests/lj-1
| 004-oom-error-frame.test.lua 
| TAP version 13
| 1..1
| ok - on-trace error handled successfully
|
| real    0m5.803s
| user    0m5.006s
| sys     0m0.795s

I suggest to play a bit with this sizings and stitching, to decrease
time of waiting. But be aware! The test should fail before the commit as
for `make LuaJIT-test` command as well as for the one-line command (like
above).

> +
> +local st, _ = pcall(chomp)
> +test:ok(st == false, 'on-trace error handled successfully')

Should we also check the error type to be sure that test is still
valid? I.e. that we catch OOM, and not TABOV, for example?

<snipped>

> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun