Subject: [LONG] 1 patch for 2 bugs, 1 suggestion, and someother thing

From: [email-masked]
Date: Thu, 22 Jun 2000 15:16:12 +0200 (MEST)

Hi, there

I found 2 bugs in function expand in file fvwm/functions.c
Each command that fvwm is about to execute goes first through
this function for $anything expansion

bug number 1 :
the space needed for string expansion might be underestimated when $$ is
followed by a variable name that expand to a text shorter than its name.
the malloc'ed buffer is too small. fvwm segv/core dump. The fix is in the
patch. Here is how to reproduce it :

>From within FvwmTalk, send something like this :
AddtoMenu Foo "zap me" Exec exec xterm -e sh -c "echo \$$[desk.width] == \`$[desk.width]' \$$[desk.height] == \`$[desk.height]' ; echo done ; read dummy "

bug number 2 :
$y expansion is performed but output index is not updated, and the expansion
is lost

To see the bug try this :
sendtomodule FvwmTalk $$y is '$y', $$x is '$x'.
from FvwmTalk, and you'll get for example
Msg: $y is '' $x '2560'

Index: functions.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/functions.c,v
retrieving revision 1.146
diff -u -u -r1.146 functions.c
--- functions.c	2000/06/15 14:30:57	1.146
+++ functions.c	2000/06/22 08:25:13
@@ -487,6 +487,11 @@
     {
       switch (input[i+1])
       {
+      case '$':
+	/* We just found $$ we must skip the 2 '$' otherwise a length
+	 * possibly < 0 will wrongly be calculated */
+	 ++i;
+      break;
       case '[':
 	/* extended variables */
 	var = &input[i+2];
@@ -673,6 +678,7 @@
 	break;
       case 'y':
 	sprintf(&out[j], "%d", Scr.Vy);
+	j += strlen(&out[j]);
 	i++;
 	break;
 
The suggestion :
In the same function I saw many lines like these ones :
	sprintf(&out[j],"0x%x",(unsigned int)tmp_win->w);
	j += strlen(&out[j]);

Since sprintf returns the length of the string, it is possible to
replace the 2 lines by one line, avoiding the strlen call :

	j += sprintf(&out[j],"0x%x",(unsigned int)tmp_win->w);

So I suggest the following patch instead of the previous one, unless
you are aware of some broken libc where sprintf won't work as it
should.

Index: functions.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/functions.c,v
retrieving revision 1.146
diff -u -u -r1.146 functions.c
--- functions.c	2000/06/15 14:30:57	1.146
+++ functions.c	2000/06/22 08:35:59
@@ -487,6 +487,11 @@
     {
       switch (input[i+1])
       {
+      case '$':
+	/* We just found $$ we must skip the 2 '$' otherwise a length
+	 * possibly < 0 will wrongly be calculated */
+	 ++i;
+      break;
       case '[':
 	/* extended variables */
 	var = &input[i+2];
@@ -655,24 +660,21 @@
 	break;
       case 'w':
 	if(tmp_win)
-	  sprintf(&out[j],"0x%x",(unsigned int)tmp_win->w);
+	  j += sprintf(&out[j],"0x%x",(unsigned int)tmp_win->w);
 	else
-	  sprintf(&out[j],"$w");
-	j += strlen(&out[j]);
+	  j += sprintf(&out[j],"$w");
 	i++;
 	break;
       case 'd':
-	sprintf(&out[j], "%d", Scr.CurrentDesk);
-	j += strlen(&out[j]);
+	j += sprintf(&out[j], "%d", Scr.CurrentDesk);
 	i++;
 	break;
       case 'x':
-	sprintf(&out[j], "%d", Scr.Vx);
-	j += strlen(&out[j]);
+	j += sprintf(&out[j], "%d", Scr.Vx);
 	i++;
 	break;
       case 'y':
-	sprintf(&out[j], "%d", Scr.Vy);
+	j += sprintf(&out[j], "%d", Scr.Vy);
 	i++;
 	break;
 
@@ -700,8 +702,7 @@
 	is_string = True;
 	break;
       case 'v':
-	sprintf(&out[j], "%s", (Fvwm_VersionInfo) ? Fvwm_VersionInfo : "");
-	j += strlen(&out[j]);
+	j += sprintf(&out[j], "%s", (Fvwm_VersionInfo) ? Fvwm_VersionInfo : "");
 	i++;
 	break;
       case '$':

Now, the discution
I want to implement a $p parameter. $p stands in my mind as ``paste the
X11 cut buffer''. The feature was easy to add and I'm happy with menu
entries that go like this :
+ "xterm in sel dir" Exec cd $p ; exec xterm
+ "Man for selected kw" Exec exec xterm -e man $p

Now, there is that addto boolean value passed to expand(). I understand 
that when addto is true, expand() must not perform expansion...
Basically, addto is true when AddToFuncing.

There are at least 2 problems with this addto boolean value.

1)
the expansion of some parameters is controlled by this addto
($[foobar] $0 - $9) while the expansion of some other is not ($d for
example)
So a function like this one is only half dynamic
AddToFunc Toto I Nop
+	I SendToModule *alk $d $x $y $[vp.x] $[vp.y] $[vp.width] $[vp.height]

$d $x $y are expanded when the function is defined, while $[vp.etc..] are
dynamically expanded.
To see the effects :
run FvwmTalk and then change desktop, change page, each time you change run
the Toto function.

2)
addto is normally a parameter of the expand function but, expand
decide to set addto to true if the commands starts with '+' and previous
command was and AddToFunc.

3)
I had to patch again (not included) expand so that addto is false
when AddToMenuing. Otherwise 
``AddToMenu Foo "xterm in sel dir" Exec cd $p ; exec xterm''
don't work.

I suggest that :
  o)  addto is renamed to reallyexpand
  o)  expand( ) does not decide to expand or not. It just do what it
      is told to.
  o)  when reallyexpand is False, expand() acts like strcpy().
  o)  expand() should be called with reallyexpand=False whenever a Menu or a
      Function is beeing defined.
      maybe it is possible to suppress this reallyexpand boolean and simply
      not call expand.

I think the changes are limited to functions.c.

I just have a few days to fix this according to my suggestions. but
maybe the result would be worse than the current behaviour and could
break some working fvwmrc (though I don't think so).

Please let me know if you want to do something yourself, or I can start.

	Christophe
-- 
char*p="[)1++++++11+(QI1)9191991)9)1)(II1)919Y91)9)1)(AK+9;991+3*(9I1*IY991)Q1\
)(1I1))A91991)Q1)(+)1)1++9;)Q1+(";main(_){while(*p){for(_=*p-32>>3;_;--_)
putchar(' ');if(!(_=*p++-32&7))putchar('\n');else while(_--)printf("_/");}}

--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: "Paul D. Smith" <[email-masked]>
Date: 22 Jun 2000 10:22:54 -0400

%% [email-masked] writes:

  c> In the same function I saw many lines like these ones :
  c> 	sprintf(&out[j],"0x%x",(unsigned int)tmp_win->w);
  c> 	j += strlen(&out[j]);

  c> Since sprintf returns the length of the string,

This is true of C standard compliant libraries, but isn't portable.

  c> So I suggest the following patch instead of the previous one,
  c> unless you are aware of some broken libc where sprintf won't work
  c> as it should.

There are a number of such legacy systems.  They're old, but still used.
We'd have to make a conscious decision to stop supporting them, at some
point; perhaps with a configure check to be sure.

Thanks!

-- 
-------------------------------------------------------------------------------
 Paul D. Smith <[email-masked]>         Network Management Development
 "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
-------------------------------------------------------------------------------
   These are my opinions---Nortel Networks takes no responsibility for them.
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Mikhael Goikhman <[email-masked]>
Date: Fri, 23 Jun 2000 03:23:39 +0300

> I found 2 bugs in function expand in file fvwm/functions.c

Applied. Thanks.

> + "Man for selected kw" Exec exec xterm -e man $p

It seems a bit problematic to me. $p can be a multiline string, what to do
in this case, substitute EOLN with '\n', space, keep only the first line?
Also this is already possible using xselection (included in ./utils/).

All your other points are probably valid, but I would prefer Dominik to
answer them.

> 	Christophe

Regards,
Mikhael.

---

From: Brad Giaccio <[email-masked]>
Date: Fri, 23 Jun 2000 05:49:17 -0400

> All your other points are probably valid, but I would prefer Dominik to

I currentlly have a few keys bound to call scripts that 
have the following line :
set selection=`xprop -root CUT_BUFFER0 | sed 's/^.*=..\(.*\).$/\1/'`
then I can do whatever I want in the script, so your example above would be

#!/bin/csh
set selection=`xprop -root CUT_BUFFER0 | sed 's/^.*=..\(.*\).$/\1/'`
if ( -d $selection) then
	cd $selection
	exec xterm 
endif

-- 
--- There are two kinds of knowledge, you either know the answer or
			you know where to find it
		      -Kane, Johnson, and anonymous
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: [email-masked]
Date: Fri, 23 Jun 2000 17:25:26 +0200 (MEST)

Le 23 Jui, Brad Giaccio écrivait à propos de "Re: [LONG] 1 patch for 2 bugs, 1 suggestion, and someother thing"
> set selection=`xprop -root CUT_BUFFER0 | sed 's/^.*=..\(.*\).$/\1/'`
Except for the number of extra processes spawned (1 for /bin/sh (Exec),
1 for /bin/csh, 1 for xprop, 1 for /bin/sed ) you're right.
Well in fact, I got 2 answers on how to handle X11 cut buffers out of
fvwm (xprop and xselection), so there's probably no need to make fwm
code bigger. I definitely forget this idea of a $p parameter.

thanks for the tip.

	Christophe