On Thu, 13 Jul 2006, 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.


Checking the code again I see no reason to have a limit to only work with 10 arguments. The main purpose of expand_args_extended was to allow access to parameters beyond the 10th. That's why it's using parse-functions to find the selected parameters rather than looking them up in the arguments array that $0-$9 expansion does.


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.


I think that, for readability some change is needed, but I don't think it's neccessary to use GetNextToken instead of PeekToken. It just adds an extra free in an extra if. For readability it would probably be nicer to add !is_single_arg to the if (upper >= 0)

/Viktor

Reply via email to