Subject: Purify problems in fvwm2

From: [email-masked] (Paul D. Smith)
Date: 04 Nov 1998 16:40:44 -0500

Whew.  There are _SO_ many bugs in fvwm2.  What a mess.

Below are a bunch of UMR (Uninitialized Memory Read) and other bug
fixes.  In all honesty they _probably_ don't cause real problems (except
the big memory leak!), but they're broken nonetheless.

Next I'll send some info on other memory leaks which I just didn't have
time to look at/fix.  These must be fixed!  At least the major ones.

Please look carefully at the change to menus.c:MenuInteraction(); the
old behavior is definitely broken but I'm not familiar enough with the
code to know for sure that the new code is right.

1998-11-04  Paul D. Smith  <[email-masked]>

	* Fix problems discovered by Purify:

	* fvwm/menus.c (MenuInteraction): Don't try to call
	GetPopupOptions() until we know we're looking at a popup menu.

	* libs/Picture.c (LoadPicture): Free pixmaps after we create and
	translate them and they're no longer needed.

	* fvwm/misc.c (GetOneMenuPositionArgument): If the returned token
	is an empty string, don't try to parse it.

	* fvwm/module.c (SendName): Zero out the end of the array to avoid
	uninitialized mem access.

	* libs/Picture.c (CachePicture): Don't look past the end of
	allocated memory.  If someone proves that this function is slowing
	down fvwm using some hard profiling, then we can revisit, but in
	the absence of this information it's not worth the risk.

	* fvwm/decorations.c (GetOlHints): Don't free(NULL).  This is
	OK under ANSI, but not portable.

	* fvwm/fvwm.c (InitFvwmDecor): Initialize the TitleHeight field to
	0; otherwise we may use it before it's set (in LoadWindowFont).

--- fvwm/fvwm.c~	Wed Nov  4 09:24:42 1998
+++ fvwm/fvwm.c	Wed Nov  4 13:29:46 1998
@@ -1096,6 +1096,7 @@
     
     fl->HiReliefGC = NULL;
     fl->HiShadowGC = NULL;
+    fl->TitleHeight = 0;
 
 #ifdef USEDECOR
     fl->tag = NULL;
--- fvwm/decorations.c~	Sat Oct 31 06:52:48 1998
+++ fvwm/decorations.c	Wed Nov  4 15:50:09 1998
@@ -188,7 +188,8 @@
 	    t->ol_hints &= ~OL_DECOR_ICON_NAME;
 	}
 
-      XFree (hints);
+      if (hints)
+        XFree(hints);
     }
 
   if(XGetWindowProperty (dpy, t->w, _XA_OL_DECOR_ADD, 0L, 20L, False,
@@ -205,6 +206,7 @@
 	else if (hints[i] == _XA_OL_DECOR_ICON_NAME)
             t->ol_hints |= OL_DECOR_ICON_NAME;
       }
+      if (hints)
       XFree (hints);
     }
 
@@ -222,7 +224,8 @@
 	else if (hints[i] == _XA_OL_DECOR_ICON_NAME)
             t->ol_hints &= ~OL_DECOR_ICON_NAME;
       }
-      XFree (hints);
+      if (hints)
+        XFree(hints);
     }
 
 }
--- fvwm/module.c~	Sat Oct 31 06:52:49 1998
+++ fvwm/module.c	Wed Nov  4 14:49:16 1998
@@ -546,6 +546,7 @@
   body[HEADER_SIZE] = data1;
   body[HEADER_SIZE+1] = data2;
   body[HEADER_SIZE+2] = data3; 
+  body[l-1] = 0; /* Make sure the last bytes are initialized. */
   strcpy((char *)&body[HEADER_SIZE+3],name);
 
   PositiveWrite(module,(unsigned long *)body, l*sizeof(unsigned long));
--- fvwm/misc.c~	Wed Nov  4 09:24:43 1998
+++ fvwm/misc.c	Wed Nov  4 14:52:06 1998
@@ -503,7 +503,7 @@
   float factor = (float)w/100;
 
   naction = GetNextToken(action, &token);
-  if (token == NULL)
+  if (token == NULL || *token == '\0')
     return action;
   length = strlen(token);
   if (token[length-1] == 'p') {
--- fvwm/menus.c~	Wed Nov  4 09:24:42 1998
+++ fvwm/menus.c	Wed Nov  4 15:54:42 1998
@@ -731,8 +731,6 @@
 	/* we're on a menu item */
 	fOffMenuAllowed = FALSE;
 	mr = mi->mr;
-	/* get pos hints for item's action */
-	GetPopupOptions(mi,&mops);
 
 	if (mr != menu && mr != mrPopup && mr != MrPopupForMi(mi)) {
 	  /* we're on an item from a prior menu */
@@ -806,6 +804,8 @@
 	}
 	if (fPopup) {
 	  DBUG("MenuInteraction","Popping up");
+          /* get pos hints for item's action */
+          GetPopupOptions(mi,&mops);
 	  mrPopup = MrPopupForMi(mi);
 	  if (!mrPopup) {
 	    fDoMenu = FALSE;
@@ -910,9 +910,6 @@
     } /* while (TRUE) */
 
   DO_RETURN:
-  if (mi) {
-    GetPopupOptions(mi, &mops);
-  }
   if (mrPopup) {
     PopDownMenu(mrPopup);
   }
@@ -944,6 +941,7 @@
       pmiExecuteAction && *pmiExecuteAction && (*pmiExecuteAction)->action) {
     f_type = find_func_type((*pmiExecuteAction)->action);
     if (f_type == F_POPUP || f_type == F_STAYSUP || f_type == F_WINDOWLIST) {
+      GetPopupOptions(mi, &mops);
       if (!(mops.flags & MENU_SELECTINPLACE)) {
 	fIgnorePosHints = TRUE;
       } else {
--- libs/Picture.c~	Sun Nov  1 13:08:38 1998
+++ libs/Picture.c	Wed Nov  4 16:04:50 1998
@@ -104,8 +104,10 @@
       p->width = my_image.width;
       p->height = my_image.height;
       p->depth = DefaultDepthOfScreen(DefaultScreenOfDisplay(dpy));
+      XpmFreeXpmImage(&my_image);
       return p;
     }
+    XpmFreeXpmImage(&my_image);
   }
 #endif
 
@@ -138,25 +140,22 @@
 {
   char *path;
   Picture *p=PictureList;
-  int i,l;
 
   /* First find the full pathname */
   if(!(path=findIconFile(name,PixmapPath,R_OK)))
     if(!(path=findIconFile(name,IconPath,R_OK)))
       return NULL;
 
-  l=strlen(path);
-
   /* See if the picture is already cached */
   while(p)
     {
-      i=l; /* Check for matching name; backwards compare will probably find
-	      differences fastest, but is a little 'unclean' (Doesn't check
-	      length of pl->name, compares beyond end, should do no harm... */
-
-      while(i>=0 && path[i]==p->name[i])
-	i--;
-      if(i<0) /* We have found a picture with the wanted name */
+      register char *p1, *p2;
+
+      for(p1=path, p2=p->name; *p1 && *p2; ++p1, ++p2)
+	if (*p1 != *p2)
+          break;
+
+      if(!*p1 && !*p2) /* We have found a picture with the wanted name */
 	{
 	  p->count++; /* Put another weight on the picture */
 	  return p;
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
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: Wed, 4 Nov 1998 20:11:01 +0100

> code to know for sure that the new code is right.

Actually I have (re)written most of the central menu code, so
please let me *always* have a look at that stuff before
fixing anything.

> 	GetPopupOptions() until we know we're looking at a popup menu.

Correct, but I suggest to fix it in GetPopupOptions, not by moving
code around (this is all a bit touchy):

Add this in GetPopupOptions just after the declarations:

  if (!IS_POPUP_MENU_ITEM(mi))
    return;
 
> 	is an empty string, don't try to parse it.

Your fix suggests replacing unnecessary work with a memory leak!!

> +  if (token == NULL || *token == '\0')     <-- token must be freed if *token == 0
>      return action;
>    length = strlen(token);

Dominik
-- 
Dominik Vogt, Hewlett-Packard GmbH, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/14-4596, fax: 07031/14-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
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] (Paul D. Smith)
Date: 05 Nov 1998 09:28:55 -0500

%% [email-masked] writes:

  dv> Be careful if you change something:

That's why I asked people to pay special attention to this change :)

  >> GetPopupOptions() until we know we're looking at a popup menu.

  dv> Correct, but I suggest to fix it in GetPopupOptions, not by moving
  dv> code around (this is all a bit touchy):

I searched for use of the resulting mops structure and moved the call to
just before that.  That _should_ work, and did work for me in my
testing.  That code does look pretty darn touchy though :).

  dv> Add this in GetPopupOptions just after the declarations:

  dv>   if (!IS_POPUP_MENU_ITEM(mi))
  dv>     return;

OK; I thought of doing this but I didn't know the right test to make.

  >> is an empty string, don't try to parse it.

  dv> Your fix suggests replacing unnecessary work with a memory leak!!

Oops.

It wasn't just unnecessary work, though; it was an undefined memory
reference (that's why Purify picked it up).  If the string is empty then
this:

  length = strlen(token);

is broken (length is 0 and length-1 references before the beginning of
the array).

I guess the right fix is to change GetNextToken so it never returns an
empty string, but instead always returns a NULL ptr if the token is
empty.  I suppose that may break other code, but it's the right fix.
I'll look into doing this.

-- 
-------------------------------------------------------------------------------
 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.hpc.uh.edu/fvwm/>.
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: Thu, 5 Nov 1998 15:35:33 +0100

> ...

I have put it in my code and will test it today. No need for you
to do anything.

> is broken (length is 0 and length-1 references before the beginning of

Double-oops.
 
> I'll look into doing this.

No, don't touch it. Far too many functions and modules rely on this
exact behaviour. It is guaranteed that GetNextToken mallocs a string
if the input string was not NULL (roughly). If we change that, lots
of stuff will crash. It should be done someday, but not now.

Bye

Dominik