[Tarantool-patches] [PATCH luajit] Fix limit check in narrow_conv_backprop().
Sergey Bronnikov
sergeyb at tarantool.org
Tue Sep 17 13:32:23 MSK 2024
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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20240917/c94d9d4f/attachment.htm>
More information about the Tarantool-patches
mailing list