Open McDutchie opened 2 weeks ago
This bug has been in ksh since forever as well. It also happens when typeset
or other declaration command names are used in place of export
.
The problem is that export demo array
is executed as a command instead of those words being taken as values for array members.
The export
command is handled by b_readonly()
in typeset.c. When we make that abort
, this call stack results. It might help us find where things go wrong.
diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index 64880f865..d66a97c61 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -90,6 +90,7 @@ static void(*nullscan)(Namval_t*,void*);
#endif
int b_readonly(int argc,char *argv[],Shbltin_t *context)
{
+abort();
int flag;
char *command = argv[0];
struct tdata tdata;
$ lldb -- arch/darwin.arm64-64/bin/ksh -c 'typeset -a arr=( (a b c) (export demo array) )'
(lldb) target create "arch/darwin.arm64-64/bin/ksh"
Current executable set to '/usr/local/src/ksh93/ksh/arch/darwin.arm64-64/bin/ksh' (arm64).
(lldb) settings set -- target.run-args "-c" "typeset -a arr=( (a b c) (export demo array) )"
(lldb) run
Process 57938 launched: '/usr/local/src/ksh93/ksh/arch/darwin.arm64-64/bin/ksh' (arm64)
Process 57938 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
frame #0: 0x00000001a934ed38 libsystem_kernel.dylib`__pthread_kill + 8
libsystem_kernel.dylib`:
-> 0x1a934ed38 <+8>: b.lo 0x1a934ed58 ; <+40>
0x1a934ed3c <+12>: pacibsp
0x1a934ed40 <+16>: stp x29, x30, [sp, #-0x10]!
0x1a934ed44 <+20>: mov x29, sp
Target 0: (ksh) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
* frame #0: 0x00000001a934ed38 libsystem_kernel.dylib`__pthread_kill + 8
frame #1: 0x00000001a9383ee0 libsystem_pthread.dylib`pthread_kill + 288
frame #2: 0x00000001a92be330 libsystem_c.dylib`abort + 168
frame #3: 0x00000001000178c0 ksh`b_readonly(argc=3, argv=0x000000010080b2e0, context=0x0000000100196210) at typeset.c:93:1
frame #4: 0x0000000100094b28 ksh`sh_exec(t=0x000000010080b250, flags=4) at xec.c:1271:21
frame #5: 0x000000010006cb08 ksh`nv_setlist(arg=0x000000010080b1f8, flags=8520192, typ=0x0000000000000000) at name.c:564:5
frame #6: 0x000000010006c4f0 ksh`nv_setlist(arg=0x000000010080b098, flags=8520192, typ=0x0000000000000000) at name.c:491:7
frame #7: 0x0000000100093eac ksh`sh_exec(t=0x000000010080b040, flags=5) at xec.c:1077:7
frame #8: 0x0000000100020d90 ksh`exfile(iop=0x0000600003500420, fno=-1) at main.c:595:4
frame #9: 0x000000010001fe98 ksh`sh_main(ac=3, av=0x000000016fdff678, userinit=0x0000000000000000) at main.c:348:2
frame #10: 0x000000010000464c ksh`main(argc=3, argv=0x000000016fdff678) at pmain.c:41:9
frame #11: 0x00000001001e108c dyld`start + 520
(lldb)
So, the first thing that stands out is that nv_setlist
calls nv_setlist
, see frames 6 and 5. On line 491, flags
is passed on to the recursive call, minus any NV_STATIC
bit.
However, on line 262, the NV_IARRAY
bit, which indicates an indexed array, is cleared from flags
, after having been saved in the array
variable on line 230. This means the recursive call on line 491 is not passing on the NV_IARRAY
bit, so the nested parentheses are not (properly) treated as containing indexed array values. This then incorrectly allows declaration commands to be executed, as if we were dealing with a compound assignment.
This patch should fix that, and (in my testing) fixes the reproducer:
diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
index 25d383b87..72b42cbbc 100644
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -488,7 +488,7 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
if(!(array&NV_IARRAY) && !(tp->com.comset->argflag&ARG_MESSAGE))
nv_setarray(np,nv_associative);
}
- nv_setlist(tp->com.comset,flags&~NV_STATIC,0);
+ nv_setlist(tp->com.comset, flags & ~NV_STATIC | array & NV_IARRAY, 0);
sh.prefix = prefix;
if(tp->com.comset->argval[1]!='[')
nv_setvtree(np);
Unfortunately, while the patch fixes the reproducer, it also causes regressions in compound variable handling.
test comvar begins at 2024-11-05+20:35:13
comvar.sh[542]: FAIL: typeset -p with nested compound indexed array not working
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvar.sh[587]: eval: syntax error at line 1: `)' unexpected
comvar.sh[588]: FAIL: print -C with multidimensional array not working
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvar.sh[589]: eval: syntax error at line 2: `)' unexpected
comvar.sh[590]: FAIL: print -v with multidimensional array not working
comvar.sh[599]: FAIL: typeset -m not working with compound -a variable
comvar.sh[631]: FAIL: moving compound indexed array element to a compound associative array element fails
test comvar failed at 2024-11-05+20:35:13 with exit code 5 [ 104 tests 5 errors ]
test comvario begins at 2024-11-05+20:35:13
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvario.sh[407]: test6[398]: read: syntax error at line 409: `)' unexpected
comvario.sh[398]: FAIL: test2/18: read -C val failed with exit code 3
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/comvario.sh[407]: test6: line 400: aa: parameter not set
test comvario failed at 2024-11-05+20:35:15 with exit code 1 [ 72 tests 1 error ]
Thankfully, compound variable assignments are conveniently flagged with the NV_COMVAR
bit. So we need to avoid passing the NV_IARRAY
flag if the indexed array is part of a compound assignment. Patch version two passes all the current regression tests, but we should try testing it for any bugs that might not be caught by current tests.
diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
index 25d383b87..ceaad5161 100644
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -478,6 +478,7 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
{
if(tp->tre.tretyp!=TLST && !tp->com.comnamp && tp->com.comset && tp->com.comset->argval[0]==0 && tp->com.comset->argchn.ap)
{
+ int multidim_iarray;
if(prefix || np)
cp = stkcopy(sh.stk,nv_name(np));
sh.prefix = cp;
@@ -488,7 +489,8 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
if(!(array&NV_IARRAY) && !(tp->com.comset->argflag&ARG_MESSAGE))
nv_setarray(np,nv_associative);
}
- nv_setlist(tp->com.comset,flags&~NV_STATIC,0);
+ multidim_iarray = (flags & NV_COMVAR) ? 0 : (array & NV_IARRAY);
+ nv_setlist(tp->com.comset, flags & ~NV_STATIC | multidim_iarray, 0);
sh.prefix = prefix;
if(tp->com.comset->argval[1]!='[')
nv_setvtree(np);
Sigh. Unfortunately, the fix is still incomplete at best:
$ arch/*/bin/ksh -c 'typeset -a arr=( (a (export b) c) (export demo array) ); typeset -p arr'
typeset -a arr=((a () c) (export demo array) )
Pre-patch output:
typeset -a arr=(([0]=a [1]=()) ())
Expected output:
typeset -a arr=( (a (export b) c) (export demo array) )
Repeating the abort() trick with the latest reproducer, we get the following call trace:
* frame #0: 0x00000001a934ed38 libsystem_kernel.dylib`__pthread_kill + 8
frame #1: 0x00000001a9383ee0 libsystem_pthread.dylib`pthread_kill + 288
frame #2: 0x00000001a92be330 libsystem_c.dylib`abort + 168
frame #3: 0x0000000100017888 ksh`b_readonly(argc=2, argv=0x00000001010098b0, context=0x0000000100196210) at typeset.c:93:1
frame #4: 0x0000000100094b28 ksh`sh_exec(t=0x0000000101009838, flags=4) at xec.c:1271:21
frame #5: 0x000000010006cb08 ksh`nv_setlist(arg=0x00000001010097e0, flags=131584, typ=0x0000000000000000) at name.c:566:5
frame #6: 0x0000000100093eac ksh`sh_exec(t=0x0000000101009788, flags=4) at xec.c:1077:7
frame #7: 0x000000010006cb08 ksh`nv_setlist(arg=0x0000000101009730, flags=8520192, typ=0x0000000000000000) at name.c:566:5
frame #8: 0x000000010006c4f0 ksh`nv_setlist(arg=0x0000000101009698, flags=8520192, typ=0x0000000000000000) at name.c:493:7
frame #9: 0x0000000100093eac ksh`sh_exec(t=0x0000000101009640, flags=4) at xec.c:1077:7
frame #10: 0x0000000100096abc ksh`sh_exec(t=0x0000000101009b10, flags=5) at xec.c:1954:5
frame #11: 0x0000000100020d58 ksh`exfile(iop=0x0000600003504370, fno=-1) at main.c:595:4
frame #12: 0x000000010001fe60 ksh`sh_main(ac=3, av=0x000000016fdff660, userinit=0x0000000000000000) at main.c:348:2
frame #13: 0x0000000100004614 ksh`main(argc=3, argv=0x000000016fdff660) at pmain.c:41:9
frame #14: 0x00000001001e108c dyld`start + 520
A declaration command name like
export
ortypeset
breaks multidimensional indexed array assignments, e.g.:Expected output:
Originally reported by @TristanJamesBall in https://github.com/ksh93/ksh/issues/790#issuecomment-2439828897