Re: [Tarantool-patches] [PATCH luajit 1/2] Prevent sanitizer warning in snap_restoredata().

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3902 bytes --]

Hi, Sergey,

thanks for the patch!

Please add a message "Also, it removes all related UBSAN suppressions, 
since they are fixed."

to commit message, like you did for the second patch.


LGTM

On 25.06.2024 18:54, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit 4a22050df9e76a28ef904382e4b4c69578973cd5)
>
> When saving FPR registers during while from a trace and restoring
> data from a snapshot, UB sanitizer produces the following warning:
> | lj_snap.c:804:32: runtime error: index 23 out of bounds for type 'intptr_t [16]'
>
> due to indexing `ex->gpr` with a fpr register, whose number is >=
> `RID_MAX_GPR`. The situation itself is harmless since this is read from
> `spill[256]` array and is rewritten in the next if branch.
>
> This patch fixes the out-of-bounds access to read from `ex->gpr` only
> conditionally.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> Relates to tarantool/tarantool#8473
> ---
>   src/lj_snap.c                                 | 13 +++------
>   ...93-out-of-bounds-snap-restoredata.test.lua | 28 +++++++++++++++++++
>   2 files changed, 32 insertions(+), 9 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1193-out-of-bounds-snap-restoredata.test.lua
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 7dc4fe35..8a33dc22 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -756,13 +756,6 @@ static void snap_restoreval(jit_State *J, GCtrace *T, ExitState *ex,
>   }
>   
>   #if LJ_HASFFI
> -# if LUAJIT_USE_UBSAN
> -/* Seehttps://github.com/LuaJIT/LuaJIT/issues/1193. */
> -static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
> -			     SnapNo snapno, BloomFilter rfilt,
> -			     IRRef ref, void *dst, CTSize sz)
> -  __attribute__((no_sanitize("bounds")));
> -# endif
>   /* Restore raw data from the trace exit state. */
>   static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
>   			     SnapNo snapno, BloomFilter rfilt,
> @@ -801,7 +794,6 @@ static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
>   	*(lua_Number *)dst = (lua_Number)*(int32_t *)dst;
>   	return;
>         }
> -      src = (int32_t *)&ex->gpr[r-RID_MIN_GPR];
>   #if !LJ_SOFTFP
>         if (r >= RID_MAX_GPR) {
>   	src = (int32_t *)&ex->fpr[r-RID_MIN_FPR];
> @@ -815,7 +807,10 @@ static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
>   #endif
>         } else
>   #endif
> -      if (LJ_64 && LJ_BE && sz == 4) src++;
> +      {
> +	src = (int32_t *)&ex->gpr[r-RID_MIN_GPR];
> +	if (LJ_64 && LJ_BE && sz == 4) src++;
> +      }
>       }
>     }
>     lj_assertJ(sz == 1 || sz == 2 || sz == 4 || sz == 8,
> diff --git a/test/tarantool-tests/lj-1193-out-of-bounds-snap-restoredata.test.lua b/test/tarantool-tests/lj-1193-out-of-bounds-snap-restoredata.test.lua
> new file mode 100644
> index 00000000..6c5fc3f6
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1193-out-of-bounds-snap-restoredata.test.lua
> @@ -0,0 +1,28 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's out-of-bounds access during
> +-- the saving of registers content in `snap_restoredata()`.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1193.
> +
> +local test = tap.test('lj-1193-out-of-bounds-snap-restoredata'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local ffi = require('ffi')
> +
> +test:plan(1)
> +
> +local double_type = ffi.typeof('double')
> +
> +jit.opt.start('hotloop=1')
> +local x = 1LL
> +for _ = 1, 4 do
> +  -- `x` is saved in the fpr register and will be restored in the
> +  -- `ex->fpr` during exit from the snapshot. But out-of-bounds
> +  -- access is happening due to indexing `ex->gpr` occasionally.
> +  x = double_type(x + 1)
> +end
> +
> +test:ok(true, 'no out-of-bounds failure')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4495 bytes --]