On Thu, Jul 13, 2006 at 05:00:27PM +0200, Viktor Griph wrote: > On Thu, 13 Jul 2006, Viktor Griph wrote: > > >On Thu, 13 Jul 2006, Dominik Vogt wrote: > > > >>On Thu, Jul 13, 2006 at 02:55:24PM +0200, Viktor Griph wrote: > >>>On Thu, 13 Jul 2006, Dominik Vogt wrote: > >>> > >>>>On Thu, Jul 13, 2006 at 09:25:09AM +0200, Viktor Griph wrote: > >>>>>On Thu, 13 Jul 2006, Scott Smedley wrote: > >>>>> > >>>>>>Hi all, > >>>>>> > >>>>>>I think there is a bug in FVWM's parameter expansion. > >>>>>>FVWM crashes with a simple command such as: > >>>>>> > >>>>>>Echo $[0] > >>>>>> > >>>>>>I am looking at this problem in GDB. The variable 'm', suddenly has a > >>>>>>huge value when I reach line 918 of fvwm/expand.c: > >>>>>> > >>>>>>if (input[m] == ']') > >>>>>> > >>>>>>Then I get a SEGV because this is an illegal memory access. > >>>>>> > >>>>>>Can anyone else confirm this problem? > >>>>>> > >>>>> > >>>>>I can'tr make it crash with just Echo $[0]. However the following make > >>>>>it > >>>>>crash 100%: > >>>>> > >>>>>AddToFunc TestFunc I Echo $[0] > >>>>>TestFunc $[0] > >>>>> > >>>>>I'll investigate that further after breakfast to see if it's the same > >>>>>crash, or a different one. > >>>> > >>>>There are several bugs/crashes in expand_args_extended(): > >>>> > >>>>1) It does not check the range of the positional parameters. It > >>>> happily parses and uses for example $[999]. I didn't try it, > >>>> but I guess it causes random memory access. > >>>> > >>>I believe that SkipNTokens just stops when there is no more to consume. > >>>Then $[999] will be empty if there are less then 1000 words in the > >>>string. > >>> > >>> > >>>>2) In the "if (is_single_arg)" it uses the token returned by > >>>> PeekToken as the base for further parsing functions. This is > >>>> strictly forbidden because PeekToken returns a pointer to a > >>>> static buffer. > >>>> > >>>Not true. With 'is_single_arg' true 'upper' will always be -1, so no > >>>other > >>>parse-function will be called on the string. > >> > >>Then, why do you not write > >> > >> if (is_single_arg) > >> ... > >> *else* if (upper >= 0) > >> ... > >> > >>? > >> > > > >Probably because I didn't think of it as I added is_single_arg later to > >deal with the fact that $0 should be the same as $[0]. > > > >>-- > >> > >>Hm, now that I think about it, the new functionality deviates from > >>the way the old $0 ... worked in important ways which should be > >>fixed: > >> > >>* A range of args like $[0-1] is expanded to the original text, > >> not the unquoted form. > >> > >> I.e. in 'foo "a0" "a1" "a2"' > >> > >> $[0-1] is expanded to '"a0" "a1"' but should be expanded to > >> 'a0 a1'. > > > >Do we really want that? It prevents passing arguments on to another > >function. The way it is now (quote-persistent) is as $* has worked, and is > >essensial for pasing multiple parameters on to another function without > >risking that they split into more parameters than intended. I believe that > >the current way is a good default until the variable quote system has been > >implemented. > > > >> > >>* Currently, $* is broken too as it does not remove quoting but > >> just copies the input string. > > > >See above. > > > >> > >>* According to the man page, things like $[*] or $[3-] are > >> removed from the command line if there are no arguments. This > >> is wrong as it prevents that the string '$[*]' is passed to > >> another command. These variable should not be treated > >> specially. Currently, not even $[0] is identical to $0. > >> > > > >The old variables also work in this way, so it's only trying to mimic the > >old behaviour. All my tests make $[0] identical to $0. In what way are > >they different? > > > >>* The code is written too complicated too understand easily (as > >> you can see from the fact that I only understood about 50% of > >> it), and it's nested too deep. > >> > > > >The deepest nestings are in the actual parsing of the parameters, and > >that's hard to do without the nesting. Regarding the actual parameter > >extraction I treid to write useful comments to tell what's going on and > >why. I'm not sure what more can be done. > > > > Waiting for a responce I've done some minor cleanup as suggested, and > removed the range-checks again. One thing that still looks weird to me, > (that wrote it) is the > > argument_string = SkipSpaces( > SkipNTokens(argument_string, lower), NULL, 0);
The token parser stops after the first non-token character, e.g. a
space. In your old code, SkipSpaces was necessary, in the patch I
just committed it isn't.
> args_end = SkipNTokens(argument_string, upper - lower + 1);
> while (argument_string < args_end && isspace(*(args_end - 1)))
> {
> args_end--;
> }
>
> which seems to assume that SkipNTokens can end with any number of spaces
> before or after the next token in the list. Is that so? Or is either
> SkipSpaces or the while-loop unneeded?
If the tokens are copied dequoted, trailing spaces have to be
removed. If $[*] does not dequote, it should keep trailing
whitespace.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt, [EMAIL PROTECTED]
signature.asc
Description: Digital signature
