Declaration command names break 2D indexed array assignments
Opened this issue · 7 comments
A declaration command name like export or typeset breaks multidimensional indexed array assignments, e.g.:
$ typeset -a arr=( (a b c) (export demo array) )
$ typeset -p arr
typeset -a arr=((a b c) ())
Expected output:
typeset -a arr=((a b c) (export demo array) )
Originally reported by @TristanJamesBall in #790 (comment)
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