Closed McDutchie closed 1 year ago
The bug should be somewhere in this function: https://github.com/ksh93/ksh/blob/7a2d3564b68be537d3f68188e3f0c5df2b45dd13/src/cmd/ksh93/sh/lex.c#L1532-L1681
If you insert this debug line:
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -1578,6 +1578,7 @@ static int comsub(register Lex_t *lp, int endtok)
while(1)
{
fcgetc(c);
+error(ERROR_warn(0),"[DEBUG] c==%c/%d",c,c);
/* skip leading white space */
if(n==0 && !sh_lexstates[ST_BEGIN][c])
continue;
…then the non-buggy reproducer shows output more or less as you might expect:
$ arch/*/bin/ksh -c 'got=$( typeset -a arr=( (a b c) 1); readonly arr; typeset -p arr ); echo $got'
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==y/121
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==p/112
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==s/115
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==-/45
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==(/40
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==(/40
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==b/98
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==c/99
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==)/41
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==1/49
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==)/41
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==;/59
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==d/100
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==o/111
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==;/59
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==y/121
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==p/112
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==s/115
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==-/45
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==)/41
typeset -r -a arr=((a b c) 1)
…whereas with the buggy reproducer, something clearly goes haywire, causing some strange kind of recursion which oddly enough is not infinite, but still causes very long output:
$ arch/*/bin/ksh -c 'got=$( typeset -a arr=((a b c) 1); readonly arr; typeset -p arr ); echo $got'
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==y/121
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==p/112
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==s/115
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==-/45
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==(/40
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==g/103
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==o/111
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==y/121
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==p/112
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==s/115
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==-/45
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==(/40
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==g/103
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==o/111
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c===/61
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==t/116
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==y/121
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==p/112
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==s/115
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==-/45
... 254 repetitions (thousands of lines) deleted ...
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==a/97
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==r/114
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==)/41
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==;/59
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==e/101
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==c/99
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==h/104
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==o/111
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c== /32
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] c==/0
arch/darwin.i386-64/bin/ksh: syntax error at line 1: `(' unmatched
Here's a patch that I think fixes the issue: Edit: This is broken.
diff --git a/src/cmd/ksh93/sh/lex.c b/src/cmd/ksh93/sh/lex.c
index c9971851..8a6e6c2d 100644
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -313,6 +313,15 @@ int sh_lex(Lex_t* lp)
/* skip over characters in the current state */
state = sh_lexstates[mode];
while((n=STATE(state,c))==0);
+ /*
+ sfprintf(sfstderr,"sh_lex: mode: %d\n",mode); sfsync(sfstderr);
+ sfprintf(sfstderr,"sh_lex: oldmode(lp): %d\n",oldmode(lp)); sfsync(sfstderr);
+ sfprintf(sfstderr,"sh_lex: c: %d\n",c); sfsync(sfstderr);
+ sfprintf(sfstderr,"sh_lex: n: %d\n",n); sfsync(sfstderr);
+ sfprintf(sfstderr,"======\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"sh_lex: lp->lexd.dolparen: %d\n",lp->lexd.dolparen); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
switch(n)
{
case S_BREAK:
@@ -478,6 +487,11 @@ int sh_lex(Lex_t* lp)
}
if(fcgetc(n)>0)
fcseek(-LEN);
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_OP: n: %d\n",n); sfsync(sfstderr);
+ sfprintf(sfstderr,"sh_lex: case S_OP: state[n]: %d\n",state[n]); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
if(state[n]==S_OP || n=='#')
{
if(n==c)
@@ -486,12 +500,19 @@ int sh_lex(Lex_t* lp)
lp->lexd.docword=1;
else if(n==LPAREN)
{
+ /* don't switch to ST_NESTED if called from comsub() */
+ if(lp->lexd.dolparen)
+ return(lp->token=c);
if(lp->lex.intest)
return(c);
lp->lexd.nest=1;
lp->lastline = sh.inlineno;
lp->lexd.lex_state = ST_NESTED;
fcseek(1);
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_OP: return\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
return(sh_lex(lp));
}
c |= SYMREP;
@@ -798,7 +819,15 @@ int sh_lex(Lex_t* lp)
poplevel(lp);
fcseek(-LEN);
n = lp->digits;
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_PAR: comsub(lp,c): begin\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
wordflags |= comsub(lp,c);
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_PAR: comsub(lp,c): end\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
lp->digits = n;
continue;
case S_RBRA:
@@ -1002,6 +1031,10 @@ int sh_lex(Lex_t* lp)
continue;
case S_POP:
do_pop:
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_POP\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
if(c==RBRACE && mode==ST_NESTED && lp->lex.nestedbrace)
{
lp->lex.nestedbrace--;
@@ -1074,6 +1107,10 @@ int sh_lex(Lex_t* lp)
}
lp->lexd.paren = 1;
}
+ /*
+ sfprintf(sfstderr,"sh_lex: case S_POP: return\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
return(lp->token=LPAREN);
}
if(mode==ST_NONE)
@@ -1578,7 +1615,18 @@ static int comsub(register Lex_t *lp, int endtok)
fcseek(-LEN);
if(c==RBRACE && lp->lex.incase)
lp->lex.incase=0;
+ /*
+ sfprintf(sfstderr,"comsub: sh_lex(lp): begin\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
c=sh_lex(lp);
+ /*
+ sfprintf(sfstderr,"comsub: sh_lex(lp): end\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"comsub: c: %d\n",c); sfsync(sfstderr);
+ sfprintf(sfstderr,"comsub: lp->lexd.lex_state: %d\n",lp->lexd.lex_state); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
switch(c)
{
case LBRACE:
diff --git a/src/cmd/ksh93/sh/parse.c b/src/cmd/ksh93/sh/parse.c
index 97df31b6..23adeb92 100644
--- a/src/cmd/ksh93/sh/parse.c
+++ b/src/cmd/ksh93/sh/parse.c
@@ -1568,7 +1568,15 @@ static Shnode_t *simple(Lex_t *lexp,int flag, struct ionod *io)
lexp->aliasok = 0;
}
retry:
+ /*
+ sfprintf(sfstderr,"simple: sh_lex(lexp): begin\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
tok = sh_lex(lexp);
+ /*
+ sfprintf(sfstderr,"simple: sh_lex(lexp): end\n"); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
if(was_assign && check_array(lexp))
type = NV_ARRAY;
if(tok==LABLSYM && (flag&SH_ASSIGN))
@@ -1583,6 +1591,10 @@ static Shnode_t *simple(Lex_t *lexp,int flag, struct ionod *io)
}
if(tok==LPAREN)
{
+ /*
+ sfprintf(sfstderr,"simple: tok==LPAREN: argp->argflag&ARG_ASSIGN: %d\n",argp->argflag&ARG_ASSIGN); sfsync(sfstderr);
+ sfprintf(sfstderr,"^^^^^^\n"); sfsync(sfstderr);
+ */
if(argp->argflag&ARG_ASSIGN)
{
int intypeset = lexp->intypeset;
If you comment out the fix and uncomment the debug prints, then I hope you'll see what I was going after in this fix:
# works since space prevents switch to ST_NESTED
arch/linux.i386-64/bin/ksh -c ': $(A=( (B) C))'
# broken since comsub() does not handle the situation followed by the switch to ST_NESTED
# this leads to recursion where sh_lex() called from comsub() calls comsub() and so on
# there are no nesting $(comsubs) here and so lp->lexd.dolparen should only be incremented up to 1
arch/linux.i386-64/bin/ksh -c ': $(A=((B) C))'
# works since simple() handles the situation followed by the ST_NESTED switch
arch/linux.i386-64/bin/ksh -c 'A=((B) C)'
I couldn't figure out a way to fix this by modifying comsub()
.
@atheik I have hand tested your fix and all appears to work.
diff --git a/src/cmd/ksh93/sh/lex.c b/src/cmd/ksh93/sh/lex.c
index c9971851..7d9d67be 100644
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -486,6 +486,9 @@ int sh_lex(Lex_t* lp)
lp->lexd.docword=1;
else if(n==LPAREN)
{
+ /* don't switch to ST_NESTED if called from comsub() */
+ if(lp->lexd.dolparen)
+ return(lp->token=c);
if(lp->lex.intest)
return(c);
lp->lexd.nest=1;
@hyenias Thank you for testing! I've looked into this some more and I think this fix changes some of the sh_lex()
codepaths too much to be a correct fix. This section which is reached after the ST_NESTED
switch might be of interest:
https://github.com/ksh93/ksh/blob/490edfe/src/cmd/ksh93/sh/lex.c#L1075-L1087
The section used to be like this:
https://github.com/ksh93/ksh93-history/blob/2009-12-24/src/cmd/ksh93/sh/lex.c#L1082-L1098
I'll think about this some more.
I also could not break @atheik's fix, until I ran the ShellSpec regression tests which seems to cover some things nothing else does. It breaks the <<
bitwise shift arithmetic operator in an arithmetic expansion:
$ arch/*/bin/ksh -c 'foo=$((foo << 1))'
arch/darwin.i386-64/bin/ksh: syntax error at line 1: `<<1' here-document not contained within command substitution
There is no command substitution in that reproducer, of course. There is probably a way to amend the patch to exclude an arithmetic expansion.
@McDutchie, Good thing you ran those tests! I don't think this can be fixed by amending to my patch. I think the proper place for a fix is at the section after the switch to ST_NESTED
I highlighted at https://github.com/ksh93/ksh/issues/269#issuecomment-1133765383, but I am stuck trying to figure it out. In hindsight I'm surprised that f73d698 hasn't broken anything as it is similarly disruptive in the way that it changes some sh_lex()
codepaths.
This seems to work:
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -488,6 +488,9 @@ int sh_lex(Lex_t* lp)
lp->lexd.docword=1;
else if(n==LPAREN)
{
+ /* don't switch to ST_NESTED if we have a compound assignment in a comsub */
+ if(lp->lexd.dolparen && fcpeek(-2)=='=')
+ return(lp->token=c);
if(lp->lex.intest)
return(c);
lp->lexd.nest=1;
This section which is reached after the
ST_NESTED
switch might be of interest: https://github.com/ksh93/ksh/blob/490edfe/src/cmd/ksh93/sh/lex.c#L1075-L1087 The section used to be like this: https://github.com/ksh93/ksh93-history/blob/2009-12-24/src/cmd/ksh93/sh/lex.c#L1082-L1098
Thank you for spotting that, @atheik. Restoring these three lines from ksh 2009-12-24 also seems to fix the bug, but unfortunately causes a regression:
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -1075,6 +1075,9 @@ int sh_lex(Lex_t* lp)
return(lp->token=EXPRSYM);
}
/* backward compatibility */
+ if(lp->lexd.dolparen)
+ fcseek(-1);
+ else
{
if(lp->lexd.warn)
errormsg(SH_DICT,ERROR_warn(0),e_lexnested,sh.inlineno);
subshell.sh[486]: FAIL: 'echo $((case x in x)echo ok;esac);:)' failed -- expected 'ok', got '/usr/local/src/ksh93/ksh/arch.dev/darwin.i386-64/bin/ksh: syntax error at line 1: `)' unexpected'
Meanwhile I've tried pretty hard to break this patch and I haven't managed – it also passes all the regression tests I can throw at it. Given that we're stuck trying to figure out anything else, I think I'm just going to commit that, even if it is not the most correct fix.
That doesn't mean it's set in stone. If a better fix is found later, it can replace that one.
Also I think the only way we're going to find out about any problems with it is by subjecting it to wider testing.
This slightly tweaked version also works: we can just return(c)
.
--- a/src/cmd/ksh93/sh/lex.c
+++ b/src/cmd/ksh93/sh/lex.c
@@ -488,6 +488,10 @@ int sh_lex(Lex_t* lp)
lp->lexd.docword=1;
else if(n==LPAREN)
{
+ /* to avoid <https://github.com/ksh93/ksh/issues/269>, don't switch to
+ * ST_NESTED if we have '=((' (nested compound assignment) in a comsub */
+ if(lp->lexd.dolparen && fcpeek(-2)=='=')
+ return(c);
if(lp->lex.intest)
return(c);
lp->lexd.nest=1;
Or not. There are still problems left when a ((
doesn't immediately follow a =
:
$ arch/*/bin/ksh -c 'got=$( typeset -a arr=( ( ((a b c)1))); typeset -p arr ); echo $got'
arch/darwin.i386-64/bin/ksh: syntax error at line 1: `(' unmatched
The problem is there doesn't seem to be a way to figure out if lp->lexd.dolparen
means a command substitution or an arithmetic expansion.
New patch that uses a new flag in the lexer state struct to keep track of nested parentheses levels after encountering =(
. In the location of the previous patch, we still need to do a check for =(
since it occurs there before comsub() has a chance to detect it, but comsub() now detects it too and will flag it up by remembering the outermost level of parentheses where =(
was first encountered, so that the new patch can check that flag and also handle nested parentheses correctly.
It's a hack, but then again, so is the whole comsub skipping mechanism. I have not yet managed to break the patch below, and it passes all the regression tests (ksh, modernish, shellspec) and all the reproducers above.
(Note that count
is known never to be < 0, so I made it into an unsigned short
. The count<=0
comparisons were misleading and are better written as count==0
.)
By the way, thanks, @atheik, for your original patch - my patch above is just a tweaked/extended version of yours and I couldn't have done it without all your debugging work.
Actually, I can do better than that. I found a way to distinguish between comsub()
skipping an arithmetic expression and skipping a command substitution. Most of the comsub()
code is for skipping a command substitution, which happens if(n==endtok || off<0)
.
This allows us to simply flag that up, and not have to special-case compound assignments at all. Much cleaner.
Urgh. The latest patch is broken after all: arithmetic expressions aren't parsed as such within command substitutions, so that the shift-left operator is mistaken for a here-document redirection again.
$ arch/darwin.arm64-64/bin/ksh -c ': $( (( 1 << 2 )) )'
arch/darwin.arm64-64/bin/ksh: syntax error at line 1: `<<2' here-document not contained within command substitution
Neither of the candidate patches currently pass all these regression tests. The last one fails test 7. The previous, hackier one one fails test 4. (Without any patch, tests 1, 2, 3 and 4 fail.)
Sigh. Back to the drawing board?
Maybe not. I found a way to make the hackier patch pass all the regression tests above. The error in it was that the new hack was only applied if endtok==LPAREN, which is nonsense.
The last version of the patch passes all the regression tests I was able to come up with. Though the patch is hacky, it's also specific: it only disables the detection of ((
…))
arithmetic commands within =(
…)
within new-form command substitutions, and nowhere else. This limits the potential for unexpected side effects. So I think I'm now confident enough to commit this. Any further bugs in it are not going to be found if no one uses it.
Well, so much for that.
I don't think this bug is going away anytime soon.
OK, I'm nothing if not stubborn. This one is the previous one plus an additional flag to keep track of whether we're currently lexing an arithmetic expression. Please test. I'm going to give this a week now to find any breakage.
Hand test successful with indexed arrays. Here is the hardest iterative test I was using for double parenthesis errors:
# All hand testing passed with indexed arrays, last most complex hand testing performed
$ ksh269 -c 'r=$( i=3; ((i)) && typeset -a arr=((a b c) $((1+0)) (e f g $((i)))) || ((i-3)) && echo ${arr[$((1+1))][1]}; typeset -p arr); typeset -p r arr'
r=$'f\ntypeset -a arr=((a b c) 1 (e f g 3) )'
$
# Switching over to hand testing compound variables using double parenthesis
$ ksh269 -c 'typeset -Ca arr=((a=ah b=beh c=si)); typeset -p arr'
ksh269: syntax error at line 1: `((' unexpected
$ ksh269 -c 'typeset -Ca arr=((a=ah b=beh c=si) ); typeset -p arr'
typeset -C -a arr=((a=ah;b=beh;c=si))
$
$ ksh269 -c 'r=$( typeset -Ca arr=((a=ah b=beh c=si)); typeset -p arr); echo $r'
ksh269: syntax error at line 1: `((' unexpected
$ ksh269 -c 'r=$( typeset -Ca arr=((a=ah b=beh c=si) ); typeset -p arr); echo $r'
typeset -C -a arr=((a=ah;b=beh;c=si))
$ ksh269 -c 'r=${ typeset -Ca arr=((a=ah b=beh c=si)); typeset -p arr; }; echo $r'
ksh269: syntax error at line 1: `((' unexpected
$ ksh269 -c 'r=${ typeset -Ca arr=((a=ah b=beh c=si) ); typeset -p arr; }; echo $r'
typeset -C -a arr=((a=ah;b=beh;c=si))
# Came across a new core dump while hand keying iterations
$ ksh269 -c 'r=$(typeset -C arr=((a=ah b=beh c=si) 1 (e f g)));'
free(): invalid pointer
Aborted (core dumped)
Thanks for testing!
# Came across a new core dump while hand keying iterations $ ksh269 -c 'r=$(typeset -C arr=((a=ah b=beh c=si) 1 (e f g)));' free(): invalid pointer Aborted (core dumped)
Ha, that core dump was accidentally fixed in 4405a3527feae43fbbb72e823169a9cac8744fa1. I like it when that happens :)
As for your other failing test cases, they don't seem to have specifically to do with the command substitution parsing problem, so I think that's a different bug.
Actually, if I add a space, it still crashes :-/
$ arch/darwin.arm64-64/bin/ksh -c 'r=$(typeset -C arr=( (a=ah b=beh c=si) 1 (e f g)));'
ksh(80535,0x1010ac580) malloc: *** error for object 0x12de05e28: pointer being freed was not allocated
ksh(80535,0x1010ac580) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
Hmmmmmmm.
Maybe, just maybe, the whole command substitution business has been a gigantic red herring distracting me from looking at the actual cause of the problem all this time.
Which is: the lexer is detecting a single ((
symbol (LPAREN|SYMREP == EXPRSYM
) in places where it should be detecting two separate opening parentheses (LPAREN
followed by LPAREN
). And, as some of your reproducers above show, it does that inside and outside of command substitutions.
Now, where is it appropriate to detect ((
a.k.a. EXPRSYM
?
((
…))
. In the shell grammar, this can only occur where any other command name can occur. This happens to be mostly the same places where reserved words are allowed to occur as well (except for in
and do
).$((
…))
. For this, it is sufficient to check whether the double parentheses are preceded by a dollar sign.
In all other cases it should be detecting two separate opening parentheses, regardless of spaces.So, I am working on a completely new approach to a patch, but I haven't figured it out yet. Flags like lp->lexd.reservok
are flagged where it seems like they shouldn't be.
That train of thought did lead me to find a convenient flag lp->comp_assign
that we can check to avoid detection of EXPRSYM
in compound assignments. This fixes all the problems, but only outside of command substitutions, because that flag is set by the parser -- it requires parsing to know whether or not to set it.
So, the lexer-only command substitution reading business is not a red herring. We still need the hack to work around the hack. But we actually need both that and the new check for lp->comp_assign
, because a command substitution is only lexed at parse time, but actually parsed at run time (and thus also lexed again, without the dolparen hack).
All hand testing passed using the yet another new patch above.
Good job! 2 years of endeavoring and so many paths taken to reach this fix. Thanks to @McDutchie for your entrenched persistence on this issue and bug resolution. And thanks to @atheik for helping light the way for both of us.
This is an old bug. With this fix, long standing confusion and grief have been cleared away. As reference, when I spot checked using ksh2020 it appears it had a fix.
Thanks for re-testing, @hyenias!
As reference, when I spot checked using ksh2020 it appears it had a fix.
I hadn't checked that yet. Indeed, it fixes the syntax error bug outside of command substitutions, but not within them. So I did a little research on ksh93-history. Version 2013-05-24 adds the exact same check for the lp->comp_assign
flag as my latest patch does:
if(lp->lex.intest || lp->comp_assign)
return lp->token=c;
Nice to have it confirmed :)
This bug was found by @hyenias, here:
Note that backtick command substitutions don't have the bug either:
So, this is a parser bug that is specific to command substitutions of the
$(
…)
variety.