Re: FVWM crashes with $[0]

[Viktor Griph]

On Thu, 13 Jul 2006, Dominik Vogt wrote:

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.


The only time it removed the trailing whitespace was in ranges with closed 
upper end. It kept them for $[*] and $[2-] etc.

/Viktor