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

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

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