Re: Bug in case stmt with '('

["Bart Schaefer" <schaefer@xxxxxxxxxxxxxxxxxxxxxxx>]

On Jul 21,  6:53pm, Morris M. Siegel wrote:
} Subject: Re: Bug in case stmt with '('
}
} I don't have the time to examine how zsh's lex.c and parse.c work,
} so I can't comment technically on Bart Schaefer and Zoltan Hidvegi's
} respective patches.

Zoltan's revised patch for this is the best one so far.  It's the way
I would have approached it had I a better understanding of the effects
of the `incmdpos' and `incasepat' state variables.  However, see below.

} However, the thought occurred to me that the
} lexer and/or parser might be simplified if we revise our outlook on
} the case-statement grammar.  Instead of the syntax of each case-
} clause's prefix being
} 
} {A}	[(] pat-1 | ... | pat-n )
} 
} (which requires the troublesome analysis of deciding whether an
} initial '(' is the optional '(' of the case-clause syntax or simply
} a glob-type '(' belonging to pat-1 proper),

It doesn't currently require that analysis, so the rest of this is
mostly for benefit of zsh-workers; zsh-users can quit reading now. :-)

} why not extend the syntax to be
} 
} {B1}	pat-1 | ... | pat-n [)]
} 
} ?  Thus (presuming that normal zsh globbing is in effect), an
} initial '(' would _always_ be a glob-type parenthesis (making
} the entire case-clause prefix into one big grouped pat-1 that
} is not followed by the optional trailing ')').

This is actually very close to how it already works.  In any case where
there is an opening paren, it *is* treated as a regular glob-type paren.
The (pre-Zoltan's-patch) code simply (1) looked for a close paren, and
(2) if it didn't find one, checked whether the previously-parsed glob
pattern in fact contained balanced parens.

The bug you reported happened becase "look for a close paren" was done
by calling the lexer with the "expect command tokens" (incmdpos) state
set wrong.

All Zoltan did was rearrange the code so that the lexer state is set
correctly before looking for the next token.  In fact, a bit later on
you describe *exactly* what the (post-Zoltan's-patch) code does:

} If one were reluctant to depart so far from traditional case-statement 
} syntax as {B1} does, one could make the trailing ')' optional only
} when pat-n itself ends in ')', and to stay even more compatible with
} POSIX syntax, additionally require that n = 1.

So your suggestion would amount to omitting the check for balanced
parens, thus assuming (in the event that a closing paren is not found)
that the glob pattern is correctly formed.  This doesn't simplify the
parser in any significant way, and it delays detection of some other
malformed glob patterns.

I think we should leave it POSIX-style.

However, there is a remaining bug in Zoltan's patch.

zagzig<8> case foo in
> (foo) echo yes;;
zsh: parse error near `echo'
zagzig<9> case foo in
foo) echo yes;; 
> esac
yes
zagzig<10> case foo in
> (foo)
> echo yes;;
> esac
yes
zagzig<11> case foo in
> (foo) foo=bar echo yes;;
> esac
yes

Apparently there still needs to be at least one token between the close
paren and the first word of the actual command.

} __________________________________
} 
} By the way, my previous posting shows that with the buggy behavior
} of zsh-3.0-pre{1,3}, when executing ". case3let.ksh", the interpreter
} erroneously falls through to the next case-clause instead of exiting
} the entire case-statement (see lines 68-75 of that posting's console
} listing).

Yes, this was true.  After Zoltan's patch, it is not true any longer,
and the bogus clause generates the appropriate syntax error, aborting
the case statement.

} In addition, when executing "zsh case3let.ksh", version
} 3.0-pre3 (but not -pre1) gets a segmentation fault (see lines 62-67).
} These would seem to be problems aside from the incorrect parsing
} of the case-clause prefix.

No.  As far as I can tell, the core dump was directly related to the
improper fall-through; that is, the same wrong code-path resulted in
both problems.  It was the difference in surrounding tokens that made
the effect be a core dump in one case and merely bad behavior in the
other.

-- 
Bart Schaefer                             Brass Lantern Enterprises
http://www.well.com/user/barts            http://www.nbn.com/people/lantern

New male in /home/schaefer:
>N  2 Justin William Schaefer  Sat May 11 03:43  53/4040  "Happy Birthday"