PATCH: fix for SEGV

PATCH: fix for SEGV

[Sven Wischnowsky]

Try this:

  ...) ./zsh -f
  % setopt autocd; hash wc=/bin/wc; echo|wc

It'll give you a SEGV in `isreallycom()', trying to look at
`cn->u.name' which isn't set because for hashed commands `cn->u.cmd'
is used.

Bye
 Sven

--- os/exec.c	Fri Aug 20 15:18:07 1999
+++ Src/exec.c	Tue Aug 24 14:12:19 1999
@@ -615,12 +615,16 @@
 int
 isreallycom(Cmdnam cn)
 {
-    char fullnam[MAXCMDLEN];
+    if (cn->flags & HASHED)
+	return 1;
+    else {
+	char fullnam[MAXCMDLEN];
 
-    strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
-    strcat(fullnam, "/");
-    strcat(fullnam, cn->nam);
-    return iscom(fullnam);
+	strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
+	strcat(fullnam, "/");
+	strcat(fullnam, cn->nam);
+	return iscom(fullnam);
+    }
 }
 
 /**/

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: fix for SEGV

[Bart Schaefer]

On Aug 24,  2:16pm, Sven Wischnowsky wrote:
} Subject: PATCH: fix for SEGV
}
}   ...) ./zsh -f
}   % setopt autocd; hash wc=/bin/wc; echo|wc
} 
} It'll give you a SEGV in `isreallycom()', trying to look at
} `cn->u.name' which isn't set because for hashed commands `cn->u.cmd'
} is used.
} 
} +    if (cn->flags & HASHED)
} +	return 1;

Is that really the right fix?  I thought the whole point of isreallycom()
was to find cases where the hash table contains a mapping that doesn't
refer to a real executable.  I'm not sure it should succeed just because
the mapping was added explicitly.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: fix for SEGV

[Bart Schaefer]

On Aug 26,  9:16am, Sven Wischnowsky wrote:
} Subject: Re: PATCH: fix for SEGV
}
} Bart Schaefer wrote:
} 
} > On Aug 24,  2:16pm, Sven Wischnowsky wrote:
} > } Subject: PATCH: fix for SEGV
} > } 
} > } +    if (cn->flags & HASHED)
} > } +	return 1;
} > 
} > Is that really the right fix?
} 
} I wasn't sure either and was hoping for a reply. isreallycom() is used
} only in one place, so we could easily move the extra test there.
} 
} -	    if (hn && trycd && !isreallycom((Cmdnam)hn)) {
} +	    if (hn && trycd && !(((Cmdnam)hn)->flags & HASHED) &&
} +		!isreallycom((Cmdnam)hn)) {

Actually, I was rather thinking more of something like this (which goes
on top of 7490, which went on top of 7472):

Index: exec.c
===================================================================
@@ -617,9 +617,15 @@
 {
     char fullnam[MAXCMDLEN];
 
-    strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
-    strcat(fullnam, "/");
-    strcat(fullnam, cn->nam);
+    if (cn->flags & HASHED)
+	strcpy(fullnam, cn->u.cmd);
+    else if (!cn->u.name)
+	return 0;
+    else {
+	strcpy(fullnam, cn->u.name);
+	strcat(fullnam, "/");
+	strcat(fullnam, cn->nam);
+    }
     return iscom(fullnam);
 }
 
@@ -1685,8 +1691,7 @@
 	    char *cmdarg = (char *) peekfirst(args);
 
 	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
-	    if (hn && trycd && !(((Cmdnam)hn)->flags & HASHED) &&
-		!isreallycom((Cmdnam)hn)) {
+	    if (hn && trycd && !isreallycom((Cmdnam)hn)) {
 		cmdnamtab->removenode(cmdnamtab, cmdarg);
 		cmdnamtab->freenode(hn);
 		hn = NULL;

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: fix for SEGV

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Aug 24,  2:16pm, Sven Wischnowsky wrote:
> } Subject: PATCH: fix for SEGV
> }
> }   ...) ./zsh -f
> }   % setopt autocd; hash wc=/bin/wc; echo|wc
> } 
> } It'll give you a SEGV in `isreallycom()', trying to look at
> } `cn->u.name' which isn't set because for hashed commands `cn->u.cmd'
> } is used.
> } 
> } +    if (cn->flags & HASHED)
> } +	return 1;
> 
> Is that really the right fix?  I thought the whole point of isreallycom()
> was to find cases where the hash table contains a mapping that doesn't
> refer to a real executable.  I'm not sure it should succeed just because
> the mapping was added explicitly.

I wasn't sure either and was hoping for a reply. isreallycom() is used
only in one place, so we could easily move the extra test there.

Bye
 Sven

--- os/exec.c	Wed Aug 25 16:21:16 1999
+++ Src/exec.c	Thu Aug 26 09:14:13 1999
@@ -615,16 +615,12 @@
 int
 isreallycom(Cmdnam cn)
 {
-    if (cn->flags & HASHED)
-	return 1;
-    else {
-	char fullnam[MAXCMDLEN];
+    char fullnam[MAXCMDLEN];
 
-	strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
-	strcat(fullnam, "/");
-	strcat(fullnam, cn->nam);
-	return iscom(fullnam);
-    }
+    strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
+    strcat(fullnam, "/");
+    strcat(fullnam, cn->nam);
+    return iscom(fullnam);
 }
 
 /**/
@@ -1689,7 +1685,8 @@
 	    char *cmdarg = (char *) peekfirst(args);
 
 	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
-	    if (hn && trycd && !isreallycom((Cmdnam)hn)) {
+	    if (hn && trycd && !(((Cmdnam)hn)->flags & HASHED) &&
+		!isreallycom((Cmdnam)hn)) {
 		cmdnamtab->removenode(cmdnamtab, cmdarg);
 		cmdnamtab->freenode(hn);
 		hn = NULL;

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de