search

ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
189 stars 31 forks source link

Crash in custom types with short integer as first member #537

Closed McDutchie closed 2 years ago

McDutchie commented 2 years ago

Original report on comp.unix.shell (Usenet)

Minimal reproducer:

typeset -T Coord_t=(
    typeset -i x
)
typeset -T Job_t=(
    typeset -si BUGTRIGGER
    Coord_t pos
    set_pos() { _.pos.x=0 ;}
)
Job_t job
job.set_pos
job.set_pos

This segfaults on the second job.set_pos call. To trigger the bug, the fist member of Job_t must be a short integer type (including an enum type, which is just a short unsigned integer with syntactic sugar). In addition, that short integer type must be the first-listed member; if it is moved to the second position, the bug vanishes.

The stack trace (below) tells me that it crashes due to an invalid pointer on line 603 in nvdisc.c. Somehow, one of the pointers in the bnames array became invalid.

    #0 0x106457408 in wrap_strcmp (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x16408)
    #1 0x105c2170a in setdisc nvdisc.c:603
    #2 0x105c1d10a in nv_setdisc nvdisc.c:515
    #3 0x105c1f309 in nv_bfsearch nvdisc.c:1102
    #4 0x105cccd2e in simple parse.c:1525
    #5 0x105cc9a05 in item parse.c:1407
    #6 0x105cc9587 in term parse.c:642
    #7 0x105cc91c5 in list parse.c:613
    #8 0x105cc8462 in sh_cmd parse.c:561
    #9 0x105cc7b18 in sh_parse parse.c:438
    #10 0x105c18ba7 in exfile main.c:586
    #11 0x105c1704f in sh_main main.c:369
    #12 0x105be3f4a in main pmain.c:41
    #13 0x7fff596ee3d4 in start (libdyld.dylib:x86_64+0x163d4)
JohnoKing commented 2 years ago

This crash was introduced in ksh93u+ 2012-05-15 by this change (2012-05-15 diff):

diff --git a/src/cmd/ksh93/sh/nvtype.c b/src/cmd/ksh93/sh/nvtype.c
index f526ffea..851ca380 100644
--- a/src/cmd/ksh93/sh/nvtype.c
+++ b/src/cmd/ksh93/sh/nvtype.c
@@ -125,6 +125,8 @@ typedef struct
    short       _dshort;
    char        _cpointer;
    char        *_dpointer;
+   int32_t     _cint32_t;
+   int32_t     *_dint32_t;
 } _Align_;

 #define alignof(t) ((char*)&((_Align_*)0)->_d##t-(char*)&((_Align_*)0)->_c##t)
@@ -188,8 +190,8 @@ size_t nv_datasize(Namval_t *np, size_t *offset)
            }
            else
            {
-               a = alignof(long);
-               s = sizeof(long);
+               a = alignof(int32_t);
+               s = sizeof(int32_t);
            }
        }
    }

It might be one of the valgrind fixes noted in the changelog for this version (although the note is vague): https://github.com/ksh93/ksh/blob/0fcaff5f1d4aad044abf1c23d61d2e91a06db944/src/cmd/ksh93/RELEASE#L111 Reverting it fixes the crash when running the reproducer, but introduces another crash in types.sh when run under ASan:

$ ASAN_OPTIONS='detect_leaks=0' bin/shtests -p types                                         
#### Regression-testing /home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh ####
test types begins at 2022-09-23+14:09:00
=================================================================
==300503==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000ff4 at pc 0x556183af45ee bp 0x7fff2a977370 sp 0x7fff2a976b30
READ of size 8 at 0x602000000ff4 thread T0
    #0 0x556183af45ed in __asan_memcpy (/home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh+0x1525ed)
    #1 0x556183b5f399 in nv_mktype /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvtype.c:1139:6
    #2 0x556183c2e60c in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:619:9
    #3 0x556183cad7e8 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1051:7
    #4 0x556183b46b04 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:604:4
    #5 0x556183b42e10 in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:369:2
    #6 0x556183b40585 in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:41:9
    #7 0x7fcd5bb4528f  (/usr/lib/libc.so.6+0x2328f) (BuildId: 26c81e7e05ebaf40bac3523b7d76be0cd71fad82)
    #8 0x7fcd5bb45349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) (BuildId: 26c81e7e05ebaf40bac3523b7d76be0cd71fad82)
    #9 0x556183a388d4 in _start /build/glibc/src/glibc/csu/../sysdeps/x86_64/start.S:115

0x602000000ff4 is located 0 bytes to the right of 4-byte region [0x602000000ff0,0x602000000ff4)
allocated by thread T0 here:
    #0 0x556183af66a9 in __interceptor_malloc (/home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh+0x1546a9)
    #1 0x556183ba9984 in sh_malloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:241:13
    #2 0x556183c3e54d in nv_putval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1781:16
    #3 0x556183c4af97 in nv_newattr /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:3008:5
    #4 0x556183d4b8ae in setall /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/typeset.c:954:7
    #5 0x556183d511ae in b_typeset /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/typeset.c:567:9
    #6 0x556183cb09a7 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1261:21
    #7 0x556183cb8c44 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1944:4
    #8 0x556183c2cfb2 in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:523:5
    #9 0x556183cad7e8 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1051:7
    #10 0x556183b46b04 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:604:4
    #11 0x556183b42e10 in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:369:2
    #12 0x556183b40585 in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:41:9
    #13 0x7fcd5bb4528f  (/usr/lib/libc.so.6+0x2328f) (BuildId: 26c81e7e05ebaf40bac3523b7d76be0cd71fad82)
McDutchie commented 2 years ago

Thanks for finding and isolating that diff!

OK, I don't understand what's going on at all, except that _Align_ seems to be a dummy struct that is used to calculate alignment offsets in nv_datasize() via the alignof macro. And I've noticed that the entries added in the 2012-05-15 diff do not match the pattern of the pre-existing entries.

The _c members are all of type char. And none of the _d members except pointer are pointers, but the new _dint32_t member is a pointer. Which does not match the size of int32_t which is actually what's being queried on the line s = sizeof(int32_t);.

So, the following patch makes the new dummy struct entries match the pattern, and just like that, the crash seems to be gone.

diff --git a/src/cmd/ksh93/sh/nvtype.c b/src/cmd/ksh93/sh/nvtype.c
index 00f5b6cda..e7e43d98b 100644
--- a/src/cmd/ksh93/sh/nvtype.c
+++ b/src/cmd/ksh93/sh/nvtype.c
@@ -112,8 +112,8 @@ typedef struct
    short       _dshort;
    char        _cpointer;
    char        *_dpointer;
-   int32_t     _cint32_t;
-   int32_t     *_dint32_t;
+   char        _cint32_t;
+   int32_t     _dint32_t;
 } _Align_;

 #define alignof(t) ((char*)&((_Align_*)0)->_d##t-(char*)&((_Align_*)0)->_c##t)
McDutchie commented 2 years ago

There is another inconsistency. Short integers (typeset -si) are typed in C as int16_t, not short. On most (all?) current systems those are the same size, but it's not guaranteed.

Plus, if short and long are no longer used for calculating alignment we can delete those entries from the _Align_ struct.

So here is a slightly more comprehensive patch:

diff --git a/src/cmd/ksh93/sh/nvtype.c b/src/cmd/ksh93/sh/nvtype.c
index 00f5b6cda..d273d6455 100644
--- a/src/cmd/ksh93/sh/nvtype.c
+++ b/src/cmd/ksh93/sh/nvtype.c
@@ -106,14 +106,12 @@ typedef struct
    float       _dfloat;
    char        _cSflong_t;
    Sflong_t    _dSflong_t;
-   char        _clong;
-   long        _dlong;
-   char        _cshort;
-   short       _dshort;
+   char        _cint32_t;
+   int32_t     _dint32_t;
+   char        _cint16_t;
+   int16_t     _dint16_t;
    char        _cpointer;
    char        *_dpointer;
-   int32_t     _cint32_t;
-   int32_t     *_dint32_t;
 } _Align_;

 #define alignof(t) ((char*)&((_Align_*)0)->_d##t-(char*)&((_Align_*)0)->_c##t)
@@ -169,8 +167,8 @@ size_t nv_datasize(Namval_t *np, size_t *offset)
            }
            else if(nv_isattr(np, NV_SHORT))
            {
-               a = alignof(short);
-               s = sizeof(short);
+               a = alignof(int16_t);
+               s = sizeof(int16_t);
            }
            else
            {
McDutchie commented 2 years ago

Fix confirmed by the original bug reporter.