Re: FVWM crashes with $[0]
I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. __eae_parse_range() is screwy: sscanf(input, %u-%n, ... returns 1 when input = 0. (which is the case for $[0]) Then variable 'n' has some undefined value which causes an illegal memory access at expand.c:232 Scott. :)
Re: FVWM crashes with $[0]
On Fri, 14 Jul 2006, Scott Smedley wrote: I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. I am using current version of CVS. Scott. There is something wrong with __eae_parse_range. I'm not that good on sscanf, but I think that it will match %u-%n even for 0 and return a good number, meaning that n will have a random value. Then it might segfaut later on *input == 0 since input += n /Viktor
Re: FVWM crashes with $[0]
On Fri, Jul 14, 2006 at 09:07:21AM +0200, Viktor Griph wrote: On Fri, 14 Jul 2006, Scott Smedley wrote: I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. I am using current version of CVS. There is something wrong with __eae_parse_range. I'm not that good on sscanf, but I think that it will match %u-%n even for 0 and return a good number, meaning that n will have a random value. Yes, one big problem is that %n may or may not increase the return code by one, so if rc = scanf(..., %u-%n, ...); returns one, this can mean is parsed just the integer and stopped, or it parsed the integer, found a '-' and assigned the %n too. Fixed. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
On Fri, 14 Jul 2006, Dominik Vogt wrote: On Fri, Jul 14, 2006 at 09:07:21AM +0200, Viktor Griph wrote: On Fri, 14 Jul 2006, Scott Smedley wrote: I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. I am using current version of CVS. There is something wrong with __eae_parse_range. I'm not that good on sscanf, but I think that it will match %u-%n even for 0 and return a good number, meaning that n will have a random value. Yes, one big problem is that %n may or may not increase the return code by one, so if rc = scanf(..., %u-%n, ...); returns one, this can mean is parsed just the integer and stopped, or it parsed the integer, found a '-' and assigned the %n too. Fixed. sscanf seems to not be very restrictive when it comes to unsigned integers. It seems as if it accepts signed integers as well. At least $[-1] hang fvwm now. /Viktor
Re: FVWM crashes with $[0]
On Fri, Jul 14, 2006 at 10:18:28AM +0200, Viktor Griph wrote: On Fri, 14 Jul 2006, Dominik Vogt wrote: On Fri, Jul 14, 2006 at 09:07:21AM +0200, Viktor Griph wrote: On Fri, 14 Jul 2006, Scott Smedley wrote: I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. I am using current version of CVS. There is something wrong with __eae_parse_range. I'm not that good on sscanf, but I think that it will match %u-%n even for 0 and return a good number, meaning that n will have a random value. Yes, one big problem is that %n may or may not increase the return code by one, so if rc = scanf(..., %u-%n, ...); returns one, this can mean is parsed just the integer and stopped, or it parsed the integer, found a '-' and assigned the %n too. Fixed. sscanf seems to not be very restrictive when it comes to unsigned integers. It seems as if it accepts signed integers as well. At least $[-1] hang fvwm now. It doesn't actually hang but tries to skip 2^32-1 tokens. It's a but in Parse.c. Fixed. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
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. /Viktor
Re: FVWM crashes with $[0]
On Thu, 13 Jul 2006, 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. /Viktor Can you see if the fix I comitted for the above fixes your error as well? It's possible that it's the same error if your strlen doesn't segfault on NULL-input (and you did call Echo $[0] from within a funcion called by a trailing space or something else to form an argument string with no tokens). /Viktor
Re: FVWM crashes with $[0]
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. 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. 3) As I said in an earlier message, PeekToken never returns an empty string. Calling PeekToken(, ...) returns a NULL pointer which the code accessed later. I've fixed all three. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
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. 3) As I said in an earlier message, PeekToken never returns an empty string. Calling PeekToken(, ...) returns a NULL pointer which the code accessed later. This was the error I fixed already. /Viktor
Re: FVWM crashes with $[0]
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
Re: FVWM crashes with $[0]
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) ... ? -- 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'. * Currently, $* is broken too as it does not remove quoting but just copies the input string. * 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 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. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
On Thu, Jul 13, 2006 at 04:10:53PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: [snip] 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. Yes, I really want that. Fvwm has always worked this way and changing this would break many functions. Mikhael suggested several times to add some notion of expansion modifies to instruct fvwm to quote the result, i.e. $[quote:0] to get '0' instead of '0'. I think this is the right approach. The fact that $* works differently just means that it's broken. [snip] * 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? Heck, you're right. I still don't it this is good: destroyfunc bar addtofunc foo + i addtofunc bar i $0 If you now invoke foo echo $1 bar 0 1 2 It should print '1', but currently it prints an empty string. It's just the way the other $-Variables (like $w) work: If it can not be expanded, keep the Variable reference. Anyway, it's too risky to change now. * 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. Look at the code I've committed. It uses a subfunction to parse the parameter range and reduces the number of nesting levels by two. It also properly unquotes argument ranges. Note that I did not test anything. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
Sorry, I've just removed your latest patch. The only difference between the old and new param style is now that $* does not dequote arguments while $[*] does. I'm not sure that this is good. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
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
Re: FVWM crashes with $[0]
On Thu, Jul 13, 2006 at 05:07:17PM +0200, Dominik Vogt wrote: On Thu, Jul 13, 2006 at 04:10:53PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: [snip] 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. Yes, I really want that. Fvwm has always worked this way and changing this would break many functions. Mikhael suggested several times to add some notion of expansion modifies to instruct fvwm to quote the result, i.e. $[quote:0] to get '0' instead of '0'. I think this is the right approach. The fact that $* works differently just means that it's broken. [snip] * 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? Heck, you're right. I still don't it this is good: destroyfunc bar addtofunc foo + i addtofunc bar i $0 If you now invoke foo echo $1 bar 0 1 2 It should print '1', but currently it prints an empty string. It's just the way the other $-Variables (like $w) work: If it can not be expanded, keep the Variable reference. Anyway, it's too risky to change now. * 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. Look at the code I've committed. It uses a subfunction to parse the parameter range and reduces the number of nesting levels by two. It also properly unquotes argument ranges. Note that I did not test anything. I don't have much time now, but two random test results: Echo $[-0] should print the first argument but prints $[-0] Echo $[1-0] (or any other invalid range) should be just copied, but it is replaced with an empty string. Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
On Thu, 13 Jul 2006, Dominik Vogt wrote: On Thu, Jul 13, 2006 at 05:07:17PM +0200, Dominik Vogt wrote: On Thu, Jul 13, 2006 at 04:10:53PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: [snip] 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. Yes, I really want that. Fvwm has always worked this way and changing this would break many functions. Mikhael suggested several times to add some notion of expansion modifies to instruct fvwm to quote the result, i.e. $[quote:0] to get '0' instead of '0'. I think this is the right approach. The fact that $* works differently just means that it's broken. [snip] * 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? Heck, you're right. I still don't it this is good: destroyfunc bar addtofunc foo + i addtofunc bar i $0 If you now invoke foo echo $1 bar 0 1 2 It should print '1', but currently it prints an empty string. It's just the way the other $-Variables (like $w) work: If it can not be expanded, keep the Variable reference. Anyway, it's too risky to change now. * 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. Look at the code I've committed. It uses a subfunction to parse the parameter range and reduces the number of nesting levels by two. It also properly unquotes argument ranges. Note that I did not test anything. I don't have much time now, but two random test results: Echo $[-0] should print the first argument but prints $[-0] This construct is not in the manpage (and thus never implemented (at lest not in my version. I've not checked the newcode yet.)) Echo $[1-0] (or any other invalid range) should be just copied, but it is replaced with an empty string. This worked with my version. The function should return -1 when it should be copied and 0 when it should be replaced with an empty string. /Viktor
Re: FVWM crashes with $[0]
On Thu, 13 Jul 2006, Dominik Vogt wrote: On Thu, Jul 13, 2006 at 04:10:53PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: [snip] 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. Yes, I really want that. Fvwm has always worked this way and changing this would break many functions. Mikhael suggested several times to add some notion of expansion modifies to instruct fvwm to quote the result, i.e. $[quote:0] to get '0' instead of '0'. I think this is the right approach. The fact that $* works differently just means that it's broken. $* has always worked they way it works as well. I for sure have some functions that rely on passing arguments to other functions, so changing the behaviour of $* would break my config. Also reading the todo-vars file it states that $[number] should be unquoted for backwardscompability. The $[range] can be treated differently as that is a new feature. I see use for both the 'raw' style and the unquoted style, but as I understand the todo it really should be single-quoted as they are niether $[number] not $[range], which are the only exceptions to that rule listed. Regarding the dequoting of the positional parameters I belive the best thing would be to make a function to unquote an entire string. I'm not sure how optimized it is to peek every token just to get rid of the quotings. Also the most flexible code for the argument expansion would be keeping the raw string as long as possible, and then dequote it and possible requote it depending on filter-selection. Maybe the best thing is to remove the $[range] alltogether until the filter-system is ready to not add another exception to how things work. I'd really like to hear Mikhael's opinion on this. [snip] * 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? Heck, you're right. I still don't it this is good: destroyfunc bar addtofunc foo + i addtofunc bar i $0 If you now invoke foo echo $1 bar 0 1 2 It should print '1', but currently it prints an empty string. It's just the way the other $-Variables (like $w) work: If it can not be expanded, keep the Variable reference. Anyway, it's too risky to change now. * 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. Look at the code I've committed. It uses a subfunction to parse the parameter range and reduces the number of nesting levels by two. It also properly unquotes argument ranges. Note that I did not test anything. I'll take a look at the code more later. Currenlty I've only looked at the diff to get some idea of what you are doing. /Viktor
Re: FVWM crashes with $[0]
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
Re: FVWM crashes with $[0]
On Thu, Jul 13, 2006 at 06:39:40PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: On Thu, Jul 13, 2006 at 04:10:53PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: [snip] 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. Yes, I really want that. Fvwm has always worked this way and changing this would break many functions. Mikhael suggested several times to add some notion of expansion modifies to instruct fvwm to quote the result, i.e. $[quote:0] to get '0' instead of '0'. I think this is the right approach. The fact that $* works differently just means that it's broken. $* has always worked they way it works as well. I for sure have some functions that rely on passing arguments to other functions, so changing the behaviour of $* would break my config. Also reading the todo-vars file it states that $[number] should be unquoted for backwardscompability. The $[range] can be treated differently as that is a new feature. I see use for both the 'raw' style and the unquoted style, but as I understand the todo it really should be single-quoted as they are niether $[number] not $[range], which are the only exceptions to that rule listed. Regarding the dequoting of the positional parameters I belive the best thing would be to make a function to unquote an entire string. I'm not sure how optimized it is to peek every token just to get rid of the quotings. Also the most flexible code for the argument expansion would be keeping the raw string as long as possible, and then dequote it and possible requote it depending on filter-selection. No, I think that is the wrong approach. Disregarding the fact that there won't be big changes in the parser before 3.0, its job is: * To read in tokens from a command line and provide them to the functions that use the parser. * To allow embedding whitespace and other special characters in tokens, the caller may quote parts of the token with double or single quotes, backticks and backslashes. * The parser has to remove one level of quoting every time it parses a token because *that is what the quoting was made for*. * If a function wants to pass a token to another command or function, it has to quote the token. The problem in fvwm is that there is no safe way to quote a token. The correct fix is to provide that functionality, not to modify the parser to do obscure things. I should never have coded the parameter $* in the way I did but should have called it $@ instead ($@ in shells works as $* now works in fvwm). Maybe the best thing is to remove the $[range] alltogether until the filter-system is ready to not add another exception to how things work. I'd really like to hear Mikhael's opinion on this. Note that passing arguments without dequoting them introduces subtle problems. Assume you have a function that takes a random number of (possibly quoted) arguments and passes them to a shell command: AddToFunc foobar + I Exec ls $* In this case, the arguments are filenames. Since the shell interprets the command, the caller of the function needs to choose a shell-compatible way of quoting. For example, he could say foobar file with spaces with double quotes but not foobar `file with spaces` with backticks. [snip] Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
On Thu, Jul 13, 2006 at 06:12:05PM +0200, Viktor Griph wrote: On Thu, 13 Jul 2006, Dominik Vogt wrote: I don't have much time now, but two random test results: Echo $[-0] should print the first argument but prints $[-0] This construct is not in the manpage (and thus never implemented (at lest not in my version. I've not checked the newcode yet.)) Well, maybe it's not worth the effort after all. Echo $[1-0] (or any other invalid range) should be just copied, but it is replaced with an empty string. This worked with my version. The function should return -1 when it should be copied and 0 when it should be replaced with an empty string. ... and it does work now that I'm comparing integers instead of pointer values ;-) Ciao Dominik ^_^ ^_^ -- Dominik Vogt, [EMAIL PROTECTED] signature.asc Description: Digital signature
Re: FVWM crashes with $[0]
I think there is a bug in FVWM's parameter expansion. FVWM crashes with a simple command such as: Echo $[0] Is this supposed to be fixed? FVWM still crashes on my system when I type Echo $[0] in FvwmConsole. I am using current version of CVS. Scott.
