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

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

[Sergey Kaplun via Tarantool-patches]

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.
+
+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)
-- 
2.46.0

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

[Sergey Bronnikov via Tarantool-patches]

[-- 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 --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 17.09.24, Sergey Bronnikov wrote:
> 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>

<snipped>

> > +-- 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.

I do not agree -- it is kind of worning to prevent changing the test.
Also, we have something similar for gh-4199-gc64-fuse, for example.

> 
> Feel free to ignore and leave as is.

So, ignoring for now.

> 
> 
> s/any more/anymore/

I decided to drop any, so the comment is the following now:
===================================================================
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
index 6bb4025d..c225fa14 100644
--- 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
@@ -15,9 +15,9 @@ test:plan(1)
 -- 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.
+-- enough times to catch the stack-buffer-overflow. I can't more
+-- simplify the reproducer any (or write it from scratch), so I
+-- leave it in that state.
 
 local DEFAULT_NUMBER = 1
 local tonumber = tonumber
===================================================================

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits below.

On Mon, Aug 26, 2024 at 03:37:40PM GMT, 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 took me a minute to understand that sentence. Let's paraphrase it the
following way:

| The function narrow_conv_backprop() does not include a limit
| check for the stack pointer (nc->sp), even though its value may
| change after several recursive calls.

> it leads to stack-buffer-overflow during the instruction narrowing. This
> patch adds a missing check.
Typo: s/a missing/the missing

>
> 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

<snipped>

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 23.09.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> 
> On Mon, Aug 26, 2024 at 03:37:40PM GMT, 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 took me a minute to understand that sentence. Let's paraphrase it the
> following way:
> 
> | The function narrow_conv_backprop() does not include a limit
> | check for the stack pointer (nc->sp), even though its value may
> | change after several recursive calls.

Rephrased as you suggested. The new commit message is the following:

| Fix limit check in narrow_conv_backprop().
|
| Thanks to Sergey Kaplun.
|
| (cherry picked from commit e45fd4cb713b610506213692f3b55a1869febb03)
|
| The function `narrow_conv_backprop()` does not include a limit check for
| the stack pointer (`nc->sp`), even though its value may change after
| several recursive calls. As a result, it leads to stack-buffer-overflow
| during the instruction narrowing. This patch adds the missing check.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#10199

> 
> > it leads to stack-buffer-overflow during the instruction narrowing. This
> > patch adds a missing check.
> Typo: s/a missing/the missing

Fixed, thanks!

> 
> >
> > 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
> 
> <snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.2 [2] and
release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714

-- 
Best regards,
Sergey Kaplun