Question: patch for memory leak

Question: patch for memory leak

[Daniel X. Pape]

Hi - 

I recently downloaded and compiled zsh-3.1.5-pws-4. However, I found a
couple of memory leaks in builtin.c where the result of a ztrdup was
being used as an argument, and not freed, and also where the result of a
"promptexpand" was being passed to "unmetafy," and then that result was
never freed.

After fixing the problem and re-compiling, I have been using the
resulting executable for two days now, and have seen no problems.

Is there any kind of test suite to use before submitting a patch? or
should I just go ahead and send it?


Thanks - 

Dan

PATCH: was Re: Question: patch for memory leak

[Daniel X. Pape]

On Thu, 14 Jan 1999, Daniel X. Pape wrote:

> I recently downloaded and compiled zsh-3.1.5-pws-4. However, I found a
> couple of memory leaks in builtin.c where the result of a ztrdup was
> being used as an argument, and not freed, and also where the result of a
> "promptexpand" was being passed to "unmetafy," and then that result was
> never freed.
> 
> After fixing the problem and re-compiling, I have been using the
> resulting executable for two days now, and have seen no problems.
> 
> Is there any kind of test suite to use before submitting a patch? or
> should I just go ahead and send it?

Here is the patch - let me know if it helps:



*** builtin.orig.c	Thu Jan 14 20:04:36 1999
--- builtin.c	Thu Jan 14 19:52:35 1999
***************
*** 1646,1651 ****
--- 1646,1652 ----
  		    setsparam(asg->name, ztrdup(asg->value));
  	    }
  	} else {
+         char *t;
  	    if (bit) {
  		if (pm->flags & PM_READONLY) {
  		    on |= ~off & PM_READONLY;
***************
*** 1657,1663 ****
  	    }
  	    /* create a new node for a parameter with the *
  	     * flags in `on' minus the readonly flag      */
! 	    pm = createparam(ztrdup(asg->name), on & ~PM_READONLY);
  	    DPUTS(!pm, "BUG: parameter not created");
  	    pm->ct = auxlen;
  	    if (func != BIN_EXPORT)
--- 1658,1666 ----
  	    }
  	    /* create a new node for a parameter with the *
  	     * flags in `on' minus the readonly flag      */
!         t = ztrdup(asg->name);
! 	    pm = createparam(t, on & ~PM_READONLY);
!         zsfree(t);
  	    DPUTS(!pm, "BUG: parameter not created");
  	    pm->ct = auxlen;
  	    if (func != BIN_EXPORT)
***************
*** 2300,2305 ****
--- 2303,2309 ----
      int *len;
      Histent ent;
      FILE *fout = stdout;
+     char *arg_to_free = 0;
  
      /* -m option -- treat the first argument as a pattern and remove
       * arguments not matching */
***************
*** 2328,2336 ****
  	    args[n] = getkeystring(args[n], &len[n],
  				    func != BIN_ECHO && !ops['e'], &nnl);
  	/* -P option -- interpret as a prompt sequence */
! 	if(ops['P'])
! 	    args[n] = unmetafy(promptexpand(metafy(args[n], len[n],
! 		META_NOALLOC), 0, NULL, NULL), &len[n]);
  	/* -D option -- interpret as a directory, and use ~ */
  	if(ops['D']) {
  	    Nameddir d = finddir(args[n]);
--- 2332,2342 ----
  	    args[n] = getkeystring(args[n], &len[n],
  				    func != BIN_ECHO && !ops['e'], &nnl);
  	/* -P option -- interpret as a prompt sequence */
!     if(ops['P']) {
!         arg_to_free = unmetafy(promptexpand(metafy(args[n], len[n],
!                           META_NOALLOC), 0, NULL, NULL), &len[n]);
!         args[n] = arg_to_free;
!     }
  	/* -D option -- interpret as a directory, and use ~ */
  	if(ops['D']) {
  	    Nameddir d = finddir(args[n]);
***************
*** 2349,2354 ****
--- 2355,2362 ----
  	PERMALLOC {
  	    pushnode(bufstack, sepjoin(args, NULL));
  	} LASTALLOC;
+     if(arg_to_free)
+         free(arg_to_free);
  	return 0;
      }
      /* -s option -- add the arguments to the history list */
***************
*** 2378,2383 ****
--- 2386,2393 ----
  	    ent->stim = ent->ftim = time(NULL);
  	    ent->flags = 0;
  	} LASTALLOC;
+     if(arg_to_free)
+         free(arg_to_free);
  	return 0;
      }
      /* -u and -p -- output to other than standard output */
***************
*** 2392,2401 ****
--- 2402,2415 ----
  	    fd = coprocout;
  	if ((fd = dup(fd)) < 0) {
  	    zwarnnam(name, "bad file number", NULL, 0);
+         if(arg_to_free)
+             free(arg_to_free);
  	    return 1;
  	}
  	if ((fout = fdopen(fd, "w")) == 0) {
  	    zwarnnam(name, "bad mode on fd", NULL, 0);
+         if(arg_to_free)
+             free(arg_to_free);
  	    return 1;
  	}
      }
***************
*** 2448,2453 ****
--- 2462,2469 ----
  	}
  	if (fout != stdout)
  	    fclose(fout);
+     if(arg_to_free)
+         free(arg_to_free);
  	return 0;
      }
      /* normal output */
***************
*** 2460,2465 ****
--- 2476,2483 ----
  	fputc(ops['N'] ? '\0' : '\n', fout);
      if (fout != stdout)
  	fclose(fout);
+     if(arg_to_free)
+         free(arg_to_free);
      return 0;
  }
  

PATCH: zsh-3.1.5-pws-4 + typeset fix: memory leak

[Peter Stephenson]

"Daniel X. Pape" wrote:
> On Thu, 14 Jan 1999, Daniel X. Pape wrote:
> 
> > I recently downloaded and compiled zsh-3.1.5-pws-4. However, I found a
> > couple of memory leaks in builtin.c where the result of a ztrdup was
> > being used as an argument, and not freed, and also where the result of a
> > "promptexpand" was being passed to "unmetafy," and then that result was
> > never freed.
> 
> Here is the patch - let me know if it helps:

You're certainly right about the memory leaks --- thanks.  I've
rewritten the patch a bit: the first part now fits in with a recent
patch of mine in zsh-workers/4902, and also I don't think you actually
need to duplicate the parameter name at all (somebody must simply have
forgotten that createparam() does that inside).  In the second case,
the problem with your approach is that `print -P' can take more than
one argument, and all need freeing: the easiest thing to do is use
heap memory (which gets freed automatically when the builtin exits),
and free the permanent memory straight away.

*** Src/builtin.c.mem	Thu Jan 14 15:55:34 1999
--- Src/builtin.c	Fri Jan 15 16:29:33 1999
***************
*** 1560,1566 ****
       * Create a new node for a parameter with the flags in `on' minus the
       * readonly flag
       */
!     pm = createparam(ztrdup(pname), on & ~PM_READONLY);
      DPUTS(!pm, "BUG: parameter not created");
      pm->ct = auxlen;
      if (func != BIN_EXPORT)
--- 1560,1566 ----
       * Create a new node for a parameter with the flags in `on' minus the
       * readonly flag
       */
!     pm = createparam(pname, on & ~PM_READONLY);
      DPUTS(!pm, "BUG: parameter not created");
      pm->ct = auxlen;
      if (func != BIN_EXPORT)
***************
*** 2346,2354 ****
  	    args[n] = getkeystring(args[n], &len[n],
  				    func != BIN_ECHO && !ops['e'], &nnl);
  	/* -P option -- interpret as a prompt sequence */
! 	if(ops['P'])
! 	    args[n] = unmetafy(promptexpand(metafy(args[n], len[n],
! 		META_NOALLOC), 0, NULL, NULL), &len[n]);
  	/* -D option -- interpret as a directory, and use ~ */
  	if(ops['D']) {
  	    Nameddir d = finddir(args[n]);
--- 2346,2362 ----
  	    args[n] = getkeystring(args[n], &len[n],
  				    func != BIN_ECHO && !ops['e'], &nnl);
  	/* -P option -- interpret as a prompt sequence */
! 	if(ops['P']) {
! 	    /*
! 	     * promptexpand uses permanent storage: to avoid
! 	     * messy memory management, stick it on the heap
! 	     * instead.
! 	     */
! 	    char *str = unmetafy(promptexpand(metafy(args[n], len[n],
! 				   META_NOALLOC), 0, NULL, NULL), &len[n]);
! 	    args[n] = dupstring(str);
! 	    free(str);
! 	}
  	/* -D option -- interpret as a directory, and use ~ */
  	if(ops['D']) {
  	    Nameddir d = finddir(args[n]);

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: PATCH: zsh-3.1.5-pws-4 + typeset fix: memory leak

[Bart Schaefer]

On Jan 15,  4:44pm, Peter Stephenson wrote:
} Subject: PATCH: zsh-3.1.5-pws-4 + typeset fix: memory leak
}
} the problem with your approach is that `print -P' can take more than
} one argument, and all need freeing: the easiest thing to do is use
} heap memory (which gets freed automatically when the builtin exits),
} and free the permanent memory straight away.

Another place where 3.0.5 appears to have been patched but 3.1.5 has not.
The 3.0.5 code reads:

	/* -P option -- interpret as a prompt sequence */
	if(ops['P']) {
	    char *arg = putprompt(metafy(args[n], len[n], META_NOALLOC),
				  &len[n], NULL, 0);
	    args[n] = (char *)alloc(len[n] + 1);
	    memcpy(args[n], arg, len[n]);
	    args[n][len[n]] = 0;
	    free(arg);
	}

I'm sure the memcpy() is done to avoid recomputing len[n]+1 in dupstring().

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