[PATCH] fix several memory leaks

[PATCH] fix several memory leaks

[Jun T.]

Here is a list of memory leaks I found and their possible
fixes. Please check the patches at the end of the post,
especially those for init.c and termcap.c/terminfo.c.

I used a command line tool named "leaks" on macOS to find
thease leaks, but I believe they can also be found (or at
least confirmed) by valgrind.

[1] found in an interactive zsh (fix: compcore.c)

    zsh% zmodload -U compinit; compinit
    zsh% setopt list_packed
    zsh% (do some completeions, e.g., ls <TAB> etc.)

Leak: 0x7fd2adc11f50  size=320  zone: DefaultMallocZone_0x105777000
    0x0000000f 0x0000000e 0x00000017 0x00000012     ................
    0x0000000d 0x00000012 0x00000008 0x00000000     ................
      (snip)
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    runhookdef module.c:1007 | list_matches compresult.c:2317 |
    runhookdef module.c:1007 | complistmatches complist.c:2017 |
    calclist compresult.c:1706 | zalloc mem.c:966 | malloc

If list_packed is set, calclist() (compresult.c:1706) allocates
'g->widths' for keeping the widths of the columns in the completion
listing. freematches() (comcpore.c) must free this also.

[2] found in B07emulate.ztst (init.c)

For leaks found during "make check", I will show a simple example
code to reproduce the same leak and the corresponding Call stack.

    zsh% emulate zsh -c 'true'
    zsh% emulate zsh -c 'true'

Leak: 0x7fea8252ff40  size=16  zone: DefaultMallocZone_0x10635a000  length: 3  "zsh"
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    execbuiltin builtin.c:505 | bin_emulate builtin.c:5960 |
    parseopts init.c:462 | ztrdup string.c:83 | zalloc mem.c:966 | malloc

The first 'emulate' allocates "zsh" at init.c:462:
        scriptname = scriptfilename = ztrdup("zsh");
The second 'emulate' does not free it and allocate "zsh" again.

I guess we need to allocate 'scriptname' only if 'toplevel' is true.
In the case of 'emulate zsh -c cmd', the 'cmd' will be passed to eval(),
and eval() save/set/restore 'scriptname' by itself, so there is no point
of setting 'scriptname' here, I think.

[3] C01arith.ztst (math.c)

    zsh% : $(( 1_0e0 ))

Leak: 0x7fafffc09200  size=16  zone: DefaultMallocZone_0x104c91000  length: 5  "10e0 "
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    matheval math.c:1464 | mathevall math.c:415 | mathparse math.c:1559 |
    zzlex math.c:824 | lexconstant math.c:538 |
    ztrdup string.c:83 | zalloc mem.c:966 | malloc

The string allocated at math.c:538 is a temporary string (ptr is reset
to nptr at line 556) and can be in heap.

[4] D04parameter.ztst (subst.c)

    zsh% a=1; b=2; echo ${a:^b}

Leak: 0x7f995de00da0  size=16  zone: DefaultMallocZone_0x10543f000
    0x5de006a0 0x00007f99 0x00000000 0x00000000     ...]............
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    stringsubst subst.c:322 | paramsubst subst.c:3172 |
    mkarray utils.c:3945 | zalloc mem.c:966 | malloc

The array allocated at subst.c:3172 can also be in heap.

[5] V07pcre.ztst (pcre.c)

    zsh% zmodload zsh/pcre
    zsh% pcre_compile 'a'
    zsh% pcre_match 'a'

Leak: 0x7fa3f250b7f0  size=16  zone: DefaultMallocZone_0x10f411000  length: 1  "a"
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    execbuiltin builtin.c:505 | bin_pcre_match pcre.c:362 |
    ztrdup string.c:83 | zalloc mem.c:966 | malloc

'plaintext' allocated at pcre.c:362 should be freed.

[6] V11db_gdbm.ztst (db_gdbm.c): three types of leaks in this test

(a) The first one is:

    zsh% zmodload zsh/db/gdbm
    zsh% ztie -d db/gdbm -f test.gdbm dbase
    zsh% zuntie dbase

Leak: 0x7fc914f00840  size=16  zone: DefaultMallocZone_0x102eea000  length: 5  "dbase"
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    bin_ztie db_gdbm.c:196 | append_tied_name db_gdbm.c:705 |
    ztrdup string.c:83 | zalloc mem.c:966 | malloc

The string allocated at db_gdbm.c:705 is not freed.
The patch adds free() in remove_tied_name(). 

(b) The other two are:

    zsh% zmodload zsh/db/gdbm
    zsh% ztie -d db/gdbm -f test.gdbm dbase
    zsh% dbase[key]=value

Leak: 0x7f8e83423620  size=16  zone: DefaultMallocZone_0x1047a3000  length: 5  "value"
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    addvars exec.c:2449 | ztrdup string.c:83 | zalloc mem.c: 966 | malloc

and
    zsh% zmodload zsh/db/gdbm
    zsh% ztie -d db/gdbm -f test.gdbm dbase
    zsh% dbase=( a a )
    zsh% dbase+=( a b )

Leak: 0x7fe681e34fa0  size=16  zone: DefaultMallocZone_0x10a3c5000  length: 1  "b"
    Call stack: [thread 0x7fffdf6f93c0]: | start | main .main.c:93 |
      (snip)
    addvars exec.c:2487 | ztrdup string.c:83 | zalloc mem.c:966 | malloc

These two have the same origin.
I think gdbmsetfn() can just save the pointer 'val' instead of
creating a copy (db_gdbm.c:362). In params.c, strsetfn() or
strvarsetfn() also saves the pointer 'x' without making a copy.

[7] V01zmodload.ztst (part1: termcap.c/terminfo.c)

There are two types of leak. The first type of them can be
reproduce by several ways, for example

    zsh% zmodload zsh/termcap
    zsh% zmodload zsh/terminfo
or
    zsh% zmodload zsh/termcap
    zsh% zmodload -u zsh/termcap
    zsh% zmodload zsh/termcap

Then may leaks are reported in the ncurses library functions.

In these modules boot_() calls setupterm(), which then allocates
memory for 'cur_term'. I think this must be freed in cleanup_()
by calling del_curterm(cur_term). Moreover, it seems the two modules
share the same 'cur_term' (a ncurses global pointer), and if
termcap has been already loaded when terminfo is being loaded,
the latter allocates another memory for 'cur_term' without
freeing the previous data. The patch below calls setupterm()
only if 'cur_term" is NULL, but I'm not 100% sure this is the
correct fix.

V01zmodload.ztst has another type of leaks, but I will discuss
it in a separate post because how to fix it (or whether it must
be fixed or not) is not clear.


diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 5f776f407..ed702b912 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -359,7 +359,7 @@ gdbmsetfn(Param pm, char *val)
     }
 
     if (val) {
-        pm->u.str = ztrdup(val);
+        pm->u.str = val;
         pm->node.flags |= PM_UPTODATE;
     }
 
@@ -732,6 +732,9 @@ static int remove_tied_name( const char *name ) {
         p++;
     }
 
+    if (*p)
+	zsfree(*p);
+
     /* Copy x+1 to x */
     while (*p) {
         *p=*(p+1);
diff --git a/Src/Modules/pcre.c b/Src/Modules/pcre.c
index 15ee34bc8..6289e003e 100644
--- a/Src/Modules/pcre.c
+++ b/Src/Modules/pcre.c
@@ -380,6 +380,7 @@ bin_pcre_match(char *nam, char **args, Options ops, UNUSED(int func))
     
     if (ovec)
 	zfree(ovec, ovecsize*sizeof(int));
+    zsfree(plaintext);
 
     return return_value;
 }
diff --git a/Src/Modules/termcap.c b/Src/Modules/termcap.c
index 60a6e138a..71257c5e9 100644
--- a/Src/Modules/termcap.c
+++ b/Src/Modules/termcap.c
@@ -353,7 +353,8 @@ boot_(UNUSED(Module m))
      * mean the modules hasn't booted---TERM may change,
      * and it should be handled dynamically---so ignore errors here.
      */
-    (void)setupterm((char *)0, 1, &errret);
+    if (!cur_term)	/* if zsh/terminfo is not loaded */
+	(void)setupterm((char *)0, 1, &errret);
 # endif
 #endif
     return  0;
@@ -363,6 +364,7 @@ boot_(UNUSED(Module m))
 int
 cleanup_(Module m)
 {
+    del_curterm(cur_term);
     return setfeatureenables(m, &module_features, NULL);
 }
 
diff --git a/Src/Modules/terminfo.c b/Src/Modules/terminfo.c
index bbd325899..7a2ef0a6f 100644
--- a/Src/Modules/terminfo.c
+++ b/Src/Modules/terminfo.c
@@ -346,7 +346,8 @@ boot_(UNUSED(Module m))
      * mean the modules hasn't booted---TERM may change,
      * and it should be handled dynamically---so ignore errors here.
      */
-    (void)setupterm((char *)0, 1, &errret);
+    if (!cur_term)	/* if zsh/termcap is not loaded */
+	(void)setupterm((char *)0, 1, &errret);
 # endif
 #endif
 
@@ -357,6 +358,7 @@ boot_(UNUSED(Module m))
 int
 cleanup_(Module m)
 {
+    del_curterm(cur_term);
     return setfeatureenables(m, &module_features, NULL);
 }
 
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index fd415da89..8eca39447 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -3556,6 +3556,8 @@ freematches(Cmgroup g, int cm)
 	    }
 	    free(g->expls);
 	}
+	if (g->widths)
+	    free(g->widths);
 	zsfree(g->name);
 	free(g);
 
diff --git a/Src/init.c b/Src/init.c
index c5372665a..e9e6be9b4 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -459,7 +459,8 @@ parseopts(char *nam, char ***argvp, char *new_opts, char **cmdp,
 		/* -c command */
 		*cmdp = *argv;
 		new_opts[INTERACTIVE] &= 1;
-		scriptname = scriptfilename = ztrdup("zsh");
+		if (toplevel)
+		    scriptname = scriptfilename = ztrdup("zsh");
 	    } else if (**argv == 'o') {
 		if (!*++*argv)
 		    argv++;
diff --git a/Src/math.c b/Src/math.c
index 32bccc6e9..4b7ecf0ab 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -535,7 +535,7 @@ lexconstant(void)
 	for (ptr2 = ptr; ptr2 < nptr; ptr2++) {
 	    if (*ptr2 == '_') {
 		int len = nptr - ptr;
-		ptr = ztrdup(ptr);
+		ptr = dupstring(ptr);
 		for (ptr2 = ptr; len; len--) {
 		    if (*ptr2 == '_')
 			chuck(ptr2);
diff --git a/Src/subst.c b/Src/subst.c
index a265a187e..c1021fbf3 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3169,7 +3169,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 		    zip = hmkarray(sval);
 	    }
 	    if (!isarr) {
-		aval = mkarray(val);
+		aval = hmkarray(val);
 		isarr = 1;
 	    }
 	    if (zip) {

Re: [PATCH] fix several memory leaks

[Jun T.]

> 2018/07/29 01:34, I wrote:
> 
> V01zmodload.ztst has another type of leaks, but I will discuss
> it in a separate post because how to fix it (or whether it must
> be fixed or not) is not clear.

Here is the other leaks in V01zmodload.zsh:

[7] V01zmodload.ztst (part2: example.c)

The second type of leaks can be reproduced by:

    zsh% zmodload -ab zsh/example example
    zsh% zmodload -u zsh/example
    zsh% builtin example
    zsh% zmodload -u zsh/example

The 'builtin example' autoloads the builtin, and then calls
bin_example(), which allocates memory for 'strparam' and
'arrparam' at lines 71 and 74 in example.c. But these are not
freed by cleanup_(). At this point 'p:exstr' and 'p:exarr' are
not enabled, and setfeatureenables() takes care of only
enabled features.

If the first 'zomdload -u' is ommited then the leak does
not occur. In this case, all the features are enabled by
'builtin example', and the last 'zmodload -u' correctly
frees the memory for 'p:exstr' and 'p:exarr'.

Do we need to fix this leak?

If builtin and parameter are independent features, then
"a builtin depending on parameters" may not be a good
"example".


The patch below is NOT a fix; it is just to confirm that
what are leaking is 'strparam' and 'arrparam'. Parameters
are supposed to be freed by setfeatureenables(,,NULL),
and explicitly freeing them in cleanup_() is not a good
example.

diff --git a/Src/Modules/example.c b/Src/Modules/example.c
index c80c9e7b2..0cae3477d 100644
--- a/Src/Modules/example.c
+++ b/Src/Modules/example.c
@@ -234,6 +234,10 @@ boot_(Module m)
 int
 cleanup_(Module m)
 {
+    zsfree(strparam);
+    strparam = NULL;
+    freearray(arrparam);
+    arrparam = NULL;
     deletewrapper(m, wrapper);
     return setfeatureenables(m, &module_features, NULL);
 }

Re: [PATCH] fix several memory leaks

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

On Sat, Jul 28, 2018, 11:06 AM Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:

>
> Parameters
> are supposed to be freed by setfeatureenables(,,NULL),
> and explicitly freeing them in cleanup_() is not a good
> example.
>

Side-effect of this module having been created before the features
mechanism existed, and incompletely updated after.

Re: [PATCH] fix several memory leaks

[Peter Stephenson]

> On 28 July 2018 at 17:34 "Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> Here is a list of memory leaks I found and their possible
> fixes. Please check the patches at the end of the post,
> especially those for init.c and termcap.c/terminfo.c.

Thanks.

> diff --git a/Src/Modules/termcap.c b/Src/Modules/termcap.c
> index 60a6e138a..71257c5e9 100644
> --- a/Src/Modules/termcap.c
> +++ b/Src/Modules/termcap.c
> @@ -353,7 +353,8 @@ boot_(UNUSED(Module m))
>       * mean the modules hasn't booted---TERM may change,
>       * and it should be handled dynamically---so ignore errors here.
>       */
> -    (void)setupterm((char *)0, 1, &errret);
> +    if (!cur_term)	/* if zsh/terminfo is not loaded */
> +	(void)setupterm((char *)0, 1, &errret);
>  # endif
>  #endif
>      return  0;

If setupterm(), over which we have no control, is really a problem we probably
need a front-end with a lock to make sure it only ever gets called once.

Something like  a function in utils.c which as the #ifdef buried in it
and simply does nothing if we don't HAVE_SETUPTERM, for example?

> diff --git a/Src/init.c b/Src/init.c
> index c5372665a..e9e6be9b4 100644
> --- a/Src/init.c
> +++ b/Src/init.c
> @@ -459,7 +459,8 @@ parseopts(char *nam, char ***argvp, char *new_opts, char **cmdp,
>  		/* -c command */
>  		*cmdp = *argv;
>  		new_opts[INTERACTIVE] &= 1;
> -		scriptname = scriptfilename = ztrdup("zsh");
> +		if (toplevel)
> +		    scriptname = scriptfilename = ztrdup("zsh");
>  	    } else if (**argv == 'o') {
>  		if (!*++*argv)
>  		    argv++;

Hmm... I'm not quite sure how this case is even supposed to work when dealing with
options not at top level.  But simply leaving the value alone, as you've done,
seems absolutely fine.

pws

Re: [PATCH] fix several memory leaks

[Jun T.]

> 2018/07/30 22:47, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> 
> If setupterm(), over which we have no control, is really a problem we probably
> need a front-end with a lock to make sure it only ever gets called once.
> 
> Something like  a function in utils.c which as the #ifdef buried in it
> and simply does nothing if we don't HAVE_SETUPTERM, for example?

Ok, it seems we need a reference count of cur_term.
How about this?


diff --git a/Src/Modules/termcap.c b/Src/Modules/termcap.c
index 60a6e138a..af4009a3a 100644
--- a/Src/Modules/termcap.c
+++ b/Src/Modules/termcap.c
@@ -345,16 +345,7 @@ int
 boot_(UNUSED(Module m))
 {
 #ifdef HAVE_TGETENT
-# ifdef HAVE_SETUPTERM
-    int errret;
-
-    /*
-     * Just because we can't set up the terminal doesn't
-     * mean the modules hasn't booted---TERM may change,
-     * and it should be handled dynamically---so ignore errors here.
-     */
-    (void)setupterm((char *)0, 1, &errret);
-# endif
+    zsetupterm();
 #endif
     return  0;
 }
@@ -363,6 +354,9 @@ boot_(UNUSED(Module m))
 int
 cleanup_(Module m)
 {
+#ifdef HAVE_TGETENT
+    zdeleteterm();
+#endif
     return setfeatureenables(m, &module_features, NULL);
 }
 
diff --git a/Src/Modules/terminfo.c b/Src/Modules/terminfo.c
index bbd325899..4596b41d2 100644
--- a/Src/Modules/terminfo.c
+++ b/Src/Modules/terminfo.c
@@ -338,16 +338,7 @@ int
 boot_(UNUSED(Module m))
 {
 #ifdef USE_TERMINFO_MODULE
-# ifdef HAVE_SETUPTERM
-    int errret;
-
-    /*
-     * Just because we can't set up the terminal doesn't
-     * mean the modules hasn't booted---TERM may change,
-     * and it should be handled dynamically---so ignore errors here.
-     */
-    (void)setupterm((char *)0, 1, &errret);
-# endif
+    zsetupterm();
 #endif
 
     return 0;
@@ -357,6 +348,9 @@ boot_(UNUSED(Module m))
 int
 cleanup_(Module m)
 {
+#ifdef USE_TERMINFO_MODULE
+    zdeleteterm();
+#endif
     return setfeatureenables(m, &module_features, NULL);
 }
 
diff --git a/Src/utils.c b/Src/utils.c
index ee2ad207f..075d27241 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -375,6 +375,43 @@ zerrmsg(FILE *file, const char *fmt, va_list ap)
     fflush(file);
 }
 
+/*
+ * Wrapper for setupterm() and del_curterm().
+ * These are called from terminfo.c and termcap.c.
+ */
+static int term_count;	/* reference count of cur_term */
+
+/**/
+mod_export void
+zsetupterm(void)
+{
+#ifdef HAVE_SETUPTERM
+    int errret;
+
+    DPUTS(term_count < 0 || (term_count > 0 && !cur_term),
+	    "inconsistent term_count and/or cur_term");
+    /*
+     * Just because we can't set up the terminal doesn't
+     * mean the modules hasn't booted---TERM may change,
+     * and it should be handled dynamically---so ignore errors here.
+     */
+    if (term_count++ == 0)
+	(void)setupterm((char *)0, 1, &errret);
+#endif
+}
+
+/**/
+mod_export void
+zdeleteterm(void)
+{
+#ifdef HAVE_SETUPTERM
+    DPUTS(term_count < 1 || !cur_term,
+	    "inconsistent term_count and/or cur_term");
+    if (--term_count == 0)
+	del_curterm(cur_term);
+#endif
+}
+
 /* Output a single character, for the termcap routines.     *
  * This is used instead of putchar since it can be a macro. */
 

Re: [PATCH] fix several memory leaks

[Peter Stephenson]

On Tue, 31 Jul 2018 23:30:33 +0900
Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
> > 2018/07/30 22:47, Peter Stephenson <p.w.stephenson@ntlworld.com>
> > wrote:
> > 
> > If setupterm(), over which we have no control, is really a problem
> > we probably need a front-end with a lock to make sure it only ever
> > gets called once.
> > 
> > Something like  a function in utils.c which as the #ifdef buried in
> > it and simply does nothing if we don't HAVE_SETUPTERM, for
> > example?  
> 
> Ok, it seems we need a reference count of cur_term.
> How about this?

That seems a reasonable solution.

Cheers
pws

Re: [PATCH] fix several memory leaks

[Jun T]

> 2018/07/29 3:57, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Sat, Jul 28, 2018, 11:06 AM Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
> 
>> 
>> Parameters
>> are supposed to be freed by setfeatureenables(,,NULL),
>> and explicitly freeing them in cleanup_() is not a good
>> example.
>> 
> 
> Side-effect of this module having been created before the features
> mechanism existed, and incompletely updated after.

If we want to show that each feature can be independently enabled, then
setting the parameter exarr (arrparam) etc. in bin_example() is better
avoided.

But we still need to make sure that the parameters are allocated only
if they are enabled. We can do this, for example, as in the patch below,
but is this a "good example"?



diff --git a/Src/Modules/example.c b/Src/Modules/example.c
index c80c9e7b2..b10c2f0e5 100644
--- a/Src/Modules/example.c
+++ b/Src/Modules/example.c
@@ -42,15 +42,14 @@ static int
 bin_example(char *nam, char **args, Options ops, UNUSED(int func))
 {
     unsigned char c;
-    char **oargs = args, **p = arrparam;
-    long i = 0;
+    char **p = arrparam;
 
     printf("Options: ");
     for (c = 32; ++c < 128;)
 	if (OPT_ISSET(ops,c))
 	    putchar(c);
     printf("\nArguments:");
-    for (; *args; i++, args++) {
+    for (; *args; args++) {
 	putchar(' ');
 	fputs(*args, stdout);
     }
@@ -66,12 +65,6 @@ bin_example(char *nam, char **args, Options ops, UNUSED(int func))
 	while (*p) printf(" %s", *p++);
     printf("\n");
 
-    intparam = i;
-    zsfree(strparam);
-    strparam = ztrdup(*oargs ? *oargs : "");
-    if (arrparam)
-	freearray(arrparam);
-    arrparam = zarrdup(oargs);
     return 0;
 }
 
@@ -221,12 +214,19 @@ enables_(Module m, int **enables)
 int
 boot_(Module m)
 {
-    intparam = 42;
-    strparam = ztrdup("example");
-    arrparam = (char **) zalloc(3 * sizeof(char *));
-    arrparam[0] = ztrdup("example");
-    arrparam[1] = ztrdup("array");
-    arrparam[2] = NULL;
+    int *enables = getfeatureenables(m, &module_features);
+
+    if (enables[5]) {
+	arrparam = (char **) zalloc(3 * sizeof(char *));
+	arrparam[0] = ztrdup("example");
+	arrparam[1] = ztrdup("array");
+	arrparam[2] = NULL;
+    }
+    if (enables[6])
+	intparam = 42;
+    if (enables[7])
+	strparam = ztrdup("example");
+
     return addwrapper(m, wrapper);
 }