Re: Patch to fix FTBFS of pvm on GNU/Hurd

[Svante Signell <svante.signell@telia.com>]

On Fri, 2011-04-15 at 01:08 +0200, Emilio Pozuelo Monfort wrote:
> On 15/04/11 00:10, Samuel Thibault wrote:
> > Svante Signell, le Thu 14 Apr 2011 23:18:32 +0200, a écrit :
> >> BTW: How to find out which packages build-depends (not depends) on
> >> pvm-dev?
> > 
> > See the graphs on my webpage
> > 
> > http://people.debian.org/~sthibault/
> > 
> > 
> >> diff -ur pvm-3.4.5/src/pvmd.c pvm-3.4.5.new/src/pvmd.c
> >> --- pvm-3.4.5/src/pvmd.c	2011-04-14 22:38:13.000000000 +0200
> >> +++ pvm-3.4.5.new/src/pvmd.c	2011-04-14 22:46:26.000000000 +0200
> >> @@ -3836,7 +3836,12 @@
> >>  	int pid;				/* task pid */
> >>  	int pfd[2];				/* pipe back from task */
> >>  	struct task *tp;		/* new task context */
> >> +
> >> +#ifndef __GNU__
> >>  	char path[MAXPATHLEN];
> >> +#else
> >> +	char *path;
> >> +#endif
> >>  	struct stat sb;
> >>  	char **ep, **eplist;
> >>  	int i;
> >> @@ -3856,10 +3861,16 @@
> >>  
> >>  	if ((tid = tid_new()) < 0) {
> >>  		pvmlogerror("forkexec() out of tids?\n");
> >> +#ifdef __GNU__
> >> +		free (path);
> >> +#endif
> >>  		return PvmOutOfRes;
> >>  	}
> >>  	if ((tp = task_new(tid)) == NULL) {
> >>  		pvmlogerror("forkexec() too many tasks?\n");
> >> +#ifdef __GNU__
> >> +		free (path);
> >> +#endif
> >>  		return PvmOutOfRes;
> >>  	}
> > 
> > Errr, why frees in these two places?  You haven't assigned any value to
> > `path' up to here, so the value is generally random and calling free()
> > is thus invalid.

Removed!

> >> @@ -3868,7 +3879,12 @@
> >>  	eplist = CINDEX(name, '/') ? nullep : epaths;
> >>  
> >>  	for (ep = eplist; *ep; ep++) {
> >> +#ifndef __GNU__
> >>  		(void)strcpy(path, *ep);
> >> +#else
> >> +		if (path = strdup(*ep) == NULL)
> >> +		  pvmlogerror("cannot allocate memory\n");
> > 
> > and call task_free(tp) and return PvmOutOfRes;

Updated, see patch.

> > Also, for now you have a leak: at each iteration of the for loop, path
> > will be overwritten by a new strdup(). You could initialize it to NULL
> > before the loop and free() it before each strdup() to fix this easily.

Updated, see patch. What kind of leak is this? path is overwritten by a
new malloced array. Are the potential problems coming from that malloc
does not clear the memory allocated, and that the new array can be
shorter than the previously allocated, leaving garbage at the end?

> And I wouldn't do #ifdef #else #endif, since strdup is portable. Just use it
> unconditionally instead of strcpy.

I chose not to use it unconditionally, then I would have to rewrite
major parts of this program. It contains very old code, first entry
1993, latest revision 2004. Since then it seems to have accumulated a
lot of patches, the debian count is 22 (23 with mine).

Still something not OK with the attached path?

--- pvm-3.4.5/src/pvmd.c.06-patched	2011-04-15 09:03:19.000000000 +0200
+++ pvm-3.4.5/src/pvmd.c	2011-04-15 15:28:08.000000000 +0200
@@ -3836,7 +3836,11 @@
 	int pid;				/* task pid */
 	int pfd[2];				/* pipe back from task */
 	struct task *tp;		/* new task context */
+#ifndef __GNU__
 	char path[MAXPATHLEN];
+#else
+	char *path = NULL;
+#endif
 	struct stat sb;
 	char **ep, **eplist;
 	int i;
@@ -3868,7 +3872,16 @@
 	eplist = CINDEX(name, '/') ? nullep : epaths;
 
 	for (ep = eplist; *ep; ep++) {
+#ifndef __GNU__
+		free (path);
 		(void)strcpy(path, *ep);
+#else
+		if (path = strdup(*ep) == NULL) {
+		  pvmlogerror("cannot allocate memory\n");
+		  task_free(tp);
+		  return PvmNoFile;
+		  }
+#endif
 		if (path[0])
 			(void)strcat(path, "/");
 		(void)strncat(path, name, sizeof(path) - strlen(path) - 1);
@@ -3939,12 +3952,18 @@
 			if (socketpair(AF_UNIX, SOCK_STREAM, 0, pfd) == -1) {
 				pvmlogperror("forkexec() socketpair");
 				task_free(tp);
+#ifdef __GNU__
+				free (path);
+#endif
 				return PvmOutOfRes;
 			}
 #else
 			if (pipe(pfd) == -1) {
 				pvmlogperror("forkexec() pipe");
 				task_free(tp);
+#ifdef __GNU__
+				free (path);
+#endif
 				return PvmOutOfRes;
 			}
 #endif
@@ -4077,6 +4096,9 @@
 				(void)close(pfd[0]);
 				(void)close(pfd[1]);
 				task_free(tp);
+#ifdef __GNU__
+				free (path);
+#endif
 				return PvmOutOfRes;
 			}
 			(void)close(pfd[1]);
@@ -4094,12 +4116,18 @@
 
 		tp->t_a_out = STRALLOC(name);
 		*tpp = tp;
+#ifdef __GNU__
+		free (path);
+#endif
 		return 0;
 	}
 	if (pvmdebmask & PDMTASK) {
 		pvmlogprintf("forkexec() didn't find <%s>\n", name);
 	}
 	task_free(tp);
+#ifdef __GNU__
+	free (path);
+#endif
 	return PvmNoFile;
 }