Re: [Tarantool-patches] [PATCH luajit] Fix limit check in narrow_conv_backprop().

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

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

Hi, Sergey,

thanks for tha patch! LGTM with a minor comment below.

On 26.08.2024 15:37, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit e45fd4cb713b610506213692f3b55a1869febb03)
>
> `narrow_conv_backprop()` misses the stack pointer (`nc->sp`) limit check
> after a bunch of recursive calls that may change its value. As a result,
> it leads to stack-buffer-overflow during the instruction narrowing. This
> patch adds a missing check.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1262-fix-limit-narrow-conv-backprop
> Related issues:
> *https://github.com/tarantool/tarantool/issues/10199
> *https://github.com/LuaJIT/LuaJIT/issues/1262
>
>   src/lj_opt_narrow.c                           |  3 +-
>   ...62-fix-limit-narrow-conv-backprop.test.lua | 61 +++++++++++++++++++
>   2 files changed, 63 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1262-fix-limit-narrow-conv-backprop.test.lua
>
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index db0da10f..6b6f20d3 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c
> @@ -341,7 +341,8 @@ static int narrow_conv_backprop(NarrowConv *nc, IRRef ref, int depth)
>         NarrowIns *savesp = nc->sp;
>         int count = narrow_conv_backprop(nc, ir->op1, depth);
>         count += narrow_conv_backprop(nc, ir->op2, depth);
> -      if (count <= 1) {  /* Limit total number of conversions. */
> +      /* Limit total number of conversions. */
> +      if (count <= 1 && nc->sp < nc->maxsp) {
>   	*nc->sp++ = NARROWINS(IRT(ir->o, nc->t), ref);
>   	return count;
>         }
> diff --git a/test/tarantool-tests/lj-1262-fix-limit-narrow-conv-backprop.test.lua b/test/tarantool-tests/lj-1262-fix-limit-narrow-conv-backprop.test.lua
> new file mode 100644
> index 00000000..6bb4025d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1262-fix-limit-narrow-conv-backprop.test.lua
> @@ -0,0 +1,61 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate stack-buffer-overflow during the
> +-- narrowing optimization.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1262.
> +
> +local test = tap.test('lj-1262-fix-limit-narrow-conv-backprop'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +-- XXX: Test fails only under ASAN.
> +-- XXX: The original reproducer was found by fuzzer:
> +--https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=70779.
> +-- It creates a long side trace with a huge amount of ADD IRs,
> +-- which are recursively used in the `narrow_conv_backprop()` many
> +-- enough times to catch the stack-buffer-overflow. I can't
> +-- simplify the reproducer any more (or write it from scratch), so
> +-- I leave it in that state.
> +

I would remove a sentence "I can't simplify the reproducer ..."

from the test, because it is useful information on review and

I think it is useless for those who will use the test.

Feel free to ignore and leave as is.


s/any more/anymore/
> +local DEFAULT_NUMBER = 1
> +local tonumber = tonumber
> +
> +local always_number = function(val)
> +  return tonumber(val) or DEFAULT_NUMBER
> +end
> +
> +local add = function(v1, v2)
> +  return always_number(v1) + always_number(v2)
> +end
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local counter_0 = 0
> +local counter_1 = 0
> +local counter_2 = 0
> +local tmp = add(nil, 'Name')
> +local Name0 = add(tmp, 'Name')
> +-- Start a long side trace here.
> +for _ = 0, 0, 0 do
> +  if counter_0 > 5 then break end
> +  counter_0 = counter_0 + 1
> +
> +  for _ = always_number(false), 1, always_number(Name0) do
> +    if counter_1 > 5 then break end
> +    counter_1 = counter_1 + 1
> +
> +    repeat
> +      if counter_2 > 5 then break end
> +      counter_2 = counter_2 + 1
> +
> +      Name0 = Name0 + Name0 + Name0
> +      Name0 = add(Name0, nil) + Name0
> +    until nil
> +  end
> +end
> +
> +test:ok(true, 'no stack-buffer-overflow during narrowing')
> +
> +test:done(true)

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