[Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by pwnhacker0x18.

(cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)

In the situation when the precision (`prec`) and amount of digits
(`hilen`) for the decimal representation are the same and `ndhi` == 0,
the `ndlo` part will become 64 (the size of the `nd` stack buffer), and
the overflow occurs.

This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
incrementation result.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9595
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1149-g-number-formating
Tarantool PR: https://github.com/tarantool/tarantool/pull/9633

The test <app-tap/gh-2717-no-quit-sigint.test.lua> fails on M1 with the
timeout (see the example [1]). This fail is patch-unrelated, since I've
obscured this failure even for the branch without sources changes (tests
only).

Related Issues:
* https://github.com/LuaJIT/LuaJIT/issues/1149
* https://github.com/tarantool/tarantool/issues/9595

[1]: https://github.com/tarantool/luajit/actions/runs/7712549489/job/21020513973#step:8:5522

Duration of failed tests (seconds):
* 60.54 app-tap/gh-2717-no-quit-sigint.test.lua

 src/lj_strfmt_num.c                           |  3 ++-
 .../lj-1149-g-number-formating-bufov.test.lua | 20 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua

diff --git a/src/lj_strfmt_num.c b/src/lj_strfmt_num.c
index c26204b7..c8d9febf 100644
--- a/src/lj_strfmt_num.c
+++ b/src/lj_strfmt_num.c
@@ -454,7 +454,8 @@ static char *lj_strfmt_wfnum(SBuf *sb, SFormat sf, lua_Number n, char *p)
 	    prec--;
 	    if (!i) {
 	      if (ndlo == ndhi) { prec = 0; break; }
-	      lj_strfmt_wuint9(tail, nd[++ndlo]);
+	      ndlo = (ndlo + 1) & 0x3f;
+	      lj_strfmt_wuint9(tail, nd[ndlo]);
 	      i = 9;
 	    }
 	  }
diff --git a/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
new file mode 100644
index 00000000..040fd5de
--- /dev/null
+++ b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
@@ -0,0 +1,20 @@
+local tap = require('tap')
+
+-- Test file to demonstrate stack-buffer-overflow in the
+-- `lj_strfmt_wfnum()` call.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1149.
+
+local test = tap.test('lj-1149-g-number-formating-bufov')
+test:plan(1)
+
+-- XXX: The test shows stack-buffer-overflow only under ASAN.
+-- The number value for the test is with the same precision
+-- (`prec` = 5) and amount of digits (`hilen` = 5) for the decimal
+-- representation. Hence, with `ndhi` == 0, the `ndlo` part will
+-- become 64 (the size of the `nd` stack buffer), and the overflow
+-- occurs.
+-- See details in the <src/lj_strfmt_num.c>:`lj_strfmt_wfnum()`.
+test:is(string.format('%7g', 0x1.144399609d407p+401), '5.5733e+120',
+        'correct format %7g result')
+
+test:done(true)
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for two nits regarding the commit message,
and two nits regarding the test case comment.
On Tue, Jan 30, 2024 at 06:04:37PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by pwnhacker0x18.
>
> (cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)
>
> In the situation when the precision (`prec`) and amount of digits
> (`hilen`) for the decimal representation are the same and `ndhi` == 0,
> the `ndlo` part will become 64 (the size of the `nd` stack buffer), and
Typo: s/will become/becomes/
> the overflow occurs.
>
> This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
> incrementation result.
Please mention that all of this happens in the `lj_strfmt_wfnum`
function in the commit message.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1149-g-number-formating
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9633
>
> The test <app-tap/gh-2717-no-quit-sigint.test.lua> fails on M1 with the
> timeout (see the example [1]). This fail is patch-unrelated, since I've
> obscured this failure even for the branch without sources changes (tests
> only).
>
> Related Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1149
> * https://github.com/tarantool/tarantool/issues/9595
>
> [1]: https://github.com/tarantool/luajit/actions/runs/7712549489/job/21020513973#step:8:5522
>
> Duration of failed tests (seconds):
> * 60.54 app-tap/gh-2717-no-quit-sigint.test.lua
>
>  src/lj_strfmt_num.c                           |  3 ++-
>  .../lj-1149-g-number-formating-bufov.test.lua | 20 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
>
> diff --git a/src/lj_strfmt_num.c b/src/lj_strfmt_num.c
> index c26204b7..c8d9febf 100644
> --- a/src/lj_strfmt_num.c
> +++ b/src/lj_strfmt_num.c
> @@ -454,7 +454,8 @@ static char *lj_strfmt_wfnum(SBuf *sb, SFormat sf, lua_Number n, char *p)
>  	    prec--;
>  	    if (!i) {
>  	      if (ndlo == ndhi) { prec = 0; break; }
> -	      lj_strfmt_wuint9(tail, nd[++ndlo]);
> +	      ndlo = (ndlo + 1) & 0x3f;
> +	      lj_strfmt_wuint9(tail, nd[ndlo]);
>  	      i = 9;
>  	    }
>  	  }
> diff --git a/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
> new file mode 100644
> index 00000000..040fd5de
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate stack-buffer-overflow in the
> +-- `lj_strfmt_wfnum()` call.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1149.
> +
> +local test = tap.test('lj-1149-g-number-formating-bufov')
> +test:plan(1)
> +
> +-- XXX: The test shows stack-buffer-overflow only under ASAN.
> +-- The number value for the test is with the same precision
Typo: s/is with/has/
> +-- (`prec` = 5) and amount of digits (`hilen` = 5) for the decimal
> +-- representation. Hence, with `ndhi` == 0, the `ndlo` part will
> +-- become 64 (the size of the `nd` stack buffer), and the overflow
Typo: s/will become/becomes/
> +-- occurs.
> +-- See details in the <src/lj_strfmt_num.c>:`lj_strfmt_wfnum()`.
> +test:is(string.format('%7g', 0x1.144399609d407p+401), '5.5733e+120',
> +        'correct format %7g result')
> +
> +test:done(true)
> --
> 2.43.0
>

Re: [Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Sergey Kaplun via Tarantool-patches]

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

On 31.01.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for two nits regarding the commit message,
> and two nits regarding the test case comment.
> On Tue, Jan 30, 2024 at 06:04:37PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by pwnhacker0x18.
> >
> > (cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)
> >
> > In the situation when the precision (`prec`) and amount of digits
> > (`hilen`) for the decimal representation are the same and `ndhi` == 0,
> > the `ndlo` part will become 64 (the size of the `nd` stack buffer), and
> Typo: s/will become/becomes/
> > the overflow occurs.
> >
> > This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
> > incrementation result.
> Please mention that all of this happens in the `lj_strfmt_wfnum`
> function in the commit message.

Fixed!
The new commit message is the following:

| Fix zero stripping in %g number formatting.
|
| Reported by pwnhacker0x18.
|
| (cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)
|
| LuaJIT uses `lj_strfmt_wfnum()` for number formatting. In the situation
| when the precision (`prec`) and amount of digits (`hilen`) for the
| decimal representation are the same and `ndhi` == 0, the `ndlo` part
| becomes 64 (the size of the `nd` stack buffer), and the overflow occurs.
|
| This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
| incrementation result.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#9595

> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9595
> > ---
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1149-g-number-formating
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/9633
> >
> > The test <app-tap/gh-2717-no-quit-sigint.test.lua> fails on M1 with the
> > timeout (see the example [1]). This fail is patch-unrelated, since I've
> > obscured this failure even for the branch without sources changes (tests
> > only).
> >
> > Related Issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/1149
> > * https://github.com/tarantool/tarantool/issues/9595
> >
> > [1]: https://github.com/tarantool/luajit/actions/runs/7712549489/job/21020513973#step:8:5522
> >
> > Duration of failed tests (seconds):
> > * 60.54 app-tap/gh-2717-no-quit-sigint.test.lua
> >

<snipped>

> > +-- XXX: The test shows stack-buffer-overflow only under ASAN.
> > +-- The number value for the test is with the same precision
> Typo: s/is with/has/
> > +-- (`prec` = 5) and amount of digits (`hilen` = 5) for the decimal
> > +-- representation. Hence, with `ndhi` == 0, the `ndlo` part will
> > +-- become 64 (the size of the `nd` stack buffer), and the overflow
> Typo: s/will become/becomes/

Fixed! See the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
index 040fd5de..b10d7b2a 100644
--- a/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
+++ b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
@@ -8,10 +8,10 @@ local test = tap.test('lj-1149-g-number-formating-bufov')
 test:plan(1)
 
 -- XXX: The test shows stack-buffer-overflow only under ASAN.
--- The number value for the test is with the same precision
+-- The number value for the test has the same precision
 -- (`prec` = 5) and amount of digits (`hilen` = 5) for the decimal
--- representation. Hence, with `ndhi` == 0, the `ndlo` part will
--- become 64 (the size of the `nd` stack buffer), and the overflow
+-- representation. Hence, with `ndhi` == 0, the `ndlo` part
+-- becomes 64 (the size of the `nd` stack buffer), and the overflow
 -- occurs.
 -- See details in the <src/lj_strfmt_num.c>:`lj_strfmt_wfnum()`.
 test:is(string.format('%7g', 0x1.144399609d407p+401), '5.5733e+120',
===================================================================

> > +-- occurs.
> > +-- See details in the <src/lj_strfmt_num.c>:`lj_strfmt_wfnum()`.
> > +test:is(string.format('%7g', 0x1.144399609d407p+401), '5.5733e+120',
> > +        'correct format %7g result')
> > +
> > +test:done(true)
> > --
> > 2.43.0
> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! LGTM

On 1/30/24 18:04, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by pwnhacker0x18.
>
> (cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)
>
> In the situation when the precision (`prec`) and amount of digits
> (`hilen`) for the decimal representation are the same and `ndhi` == 0,
> the `ndlo` part will become 64 (the size of the `nd` stack buffer), and
> the overflow occurs.
>
> This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
> incrementation result.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1149-g-number-formating
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9633
>
> The test <app-tap/gh-2717-no-quit-sigint.test.lua> fails on M1 with the
> timeout (see the example [1]). This fail is patch-unrelated, since I've
> obscured this failure even for the branch without sources changes (tests
> only).
>
> Related Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1149
> * https://github.com/tarantool/tarantool/issues/9595
>
> [1]: https://github.com/tarantool/luajit/actions/runs/7712549489/job/21020513973#step:8:5522
>
> Duration of failed tests (seconds):
> * 60.54 app-tap/gh-2717-no-quit-sigint.test.lua
>
>   src/lj_strfmt_num.c                           |  3 ++-
>   .../lj-1149-g-number-formating-bufov.test.lua | 20 +++++++++++++++++++
>   2 files changed, 22 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
>
> diff --git a/src/lj_strfmt_num.c b/src/lj_strfmt_num.c
> index c26204b7..c8d9febf 100644
> --- a/src/lj_strfmt_num.c
> +++ b/src/lj_strfmt_num.c
> @@ -454,7 +454,8 @@ static char *lj_strfmt_wfnum(SBuf *sb, SFormat sf, lua_Number n, char *p)
>   	    prec--;
>   	    if (!i) {
>   	      if (ndlo == ndhi) { prec = 0; break; }
> -	      lj_strfmt_wuint9(tail, nd[++ndlo]);
> +	      ndlo = (ndlo + 1) & 0x3f;
> +	      lj_strfmt_wuint9(tail, nd[ndlo]);
>   	      i = 9;
>   	    }
>   	  }
> diff --git a/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
> new file mode 100644
> index 00000000..040fd5de
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate stack-buffer-overflow in the
> +-- `lj_strfmt_wfnum()` call.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1149.
> +
> +local test = tap.test('lj-1149-g-number-formating-bufov')
> +test:plan(1)
> +
> +-- XXX: The test shows stack-buffer-overflow only under ASAN.
> +-- The number value for the test is with the same precision
> +-- (`prec` = 5) and amount of digits (`hilen` = 5) for the decimal
> +-- representation. Hence, with `ndhi` == 0, the `ndlo` part will
> +-- become 64 (the size of the `nd` stack buffer), and the overflow
> +-- occurs.
> +-- See details in the <src/lj_strfmt_num.c>:`lj_strfmt_wfnum()`.
> +test:is(string.format('%7g', 0x1.144399609d407p+401), '5.5733e+120',
> +        'correct format %7g result')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit] Fix zero stripping in %g number formatting.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/3.0 and
release/2.11.

On 30.01.24, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by pwnhacker0x18.
> 
> (cherry picked from commit 343ce0edaf3906a62022936175b2f5410024cbfc)
> 
> In the situation when the precision (`prec`) and amount of digits
> (`hilen`) for the decimal representation are the same and `ndhi` == 0,
> the `ndlo` part will become 64 (the size of the `nd` stack buffer), and
> the overflow occurs.
> 
> This patch adds the corresponding mask (0x3f == 63) for the `ndlo`
> incrementation result.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9595
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1149-g-number-formating
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9633
> 
> The test <app-tap/gh-2717-no-quit-sigint.test.lua> fails on M1 with the
> timeout (see the example [1]). This fail is patch-unrelated, since I've
> obscured this failure even for the branch without sources changes (tests
> only).
> 
> Related Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1149
> * https://github.com/tarantool/tarantool/issues/9595
> 
> [1]: https://github.com/tarantool/luajit/actions/runs/7712549489/job/21020513973#step:8:5522
> 
> Duration of failed tests (seconds):
> * 60.54 app-tap/gh-2717-no-quit-sigint.test.lua
> 
>  src/lj_strfmt_num.c                           |  3 ++-
>  .../lj-1149-g-number-formating-bufov.test.lua | 20 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-1149-g-number-formating-bufov.test.lua
> 

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
IM