Re: Valgrind automatic tests, ran for almost every Zsh test and zredis

[Sebastian Gniazdowski <psprint@zdharma.org>]

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

On 17 czerwca 2017 at 07:08:24, Bart Schaefer (schaefer@brasslantern.com) wrote:
> On Jun 16, 5:14pm, Sebastian Gniazdowski wrote:
> }
> } ============ The maybe-suspicious reports ============
>  
> A few of these look like they're probably the same report reached via
> slightly different script paths. The backtraces aren't very helpful
> without knowing which specific test case set them off.

I've fixed this, before each Valgrind error currently executed test case is shown (attached how it looks like).

> I'm guessing most of these are going to turn out to be from a subshell
> exec'ing or exiting without bothering to clean up allocations done by
> the parent, particularly in the case where the leaked memory is from
> the stdio library (indicating "FILE *bshin" was never fclose()d).

I muted many fork / zfork errors initially.

Today I wanted to catch something. The bicat() usage seems to leak what getoutputfile() returns:

- that function always calls: gettempname(NULL, 0), and the 0 == zalloc() memory
- in stringsubst(), after memcpy(), result of getoutputfile() is unused, free to release

Attached is a patch with fix for this, it removes the Valgrind error. The error (Valgrind stack-trace) is attached in the PNG.

I've added 2 error definitions, A04 is now fully silent:

https://github.com/zdharma/VATS-zsh

Does this maybe prove usability of VATS, and open way to add to upstream?

--  
Sebastian Gniazdowski
psprint /at/ zdharma.org

[-- Attachment #2: bicat_error.png --]
[-- Type: image/png, Size: 188769 bytes --]

[-- Attachment #3: bicat.diff.txt --]
[-- Type: text/plain, Size: 1706 bytes --]

commit 47aa822fe16e29ba9c9a698f1ba6c830081ea0b2
Author: Sebastian Gniazdowski <sgniazdowski@gmail.com>
Date:   Sun Jun 18 12:57:17 2017 +0200

    Fix a leak: stringsubst -> getoutputfile -> gettempname -> bicat

diff --git a/Src/exec.c b/Src/exec.c
index debb0ae..133e667 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4570,6 +4570,7 @@ getoutputfile(char *cmd, char **eptr)
     }
     if (!(prog = parsecmd(cmd, eptr)))
 	return NULL;
+    /* 0 - getoutputfile always uses zalloc */
     if (!(nam = gettempname(NULL, 0)))
 	return NULL;
 
diff --git a/Src/subst.c b/Src/subst.c
index 5b1bf89..c2d3aee 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -162,7 +162,7 @@ static LinkNode
 stringsubst(LinkList list, LinkNode node, int pf_flags, int *ret_flags,
 	    int asssub)
 {
-    int qt;
+    int qt, used_zalloc = 0;
     char *str3 = (char *)getdata(node);
     char *str  = str3, c;
 
@@ -175,8 +175,12 @@ stringsubst(LinkList list, LinkNode node, int pf_flags, int *ret_flags,
 
 	    if (c == Inang || c == OutangProc)
 		subst = getproc(str, &rest);	/* <(...) or >(...) */
-	    else
+	    else {
+                /* Will use zalloc (always calls gettempname(NULL, 0)), the 0 -> zalloc */
 		subst = getoutputfile(str, &rest);	/* =(...) */
+                if (subst)
+                    used_zalloc = 1;
+            }
 	    if (errflag)
 		return NULL;
 	    if (!subst)
@@ -193,6 +197,9 @@ stringsubst(LinkList list, LinkNode node, int pf_flags, int *ret_flags,
 		memcpy(sptr, subst, sublen);
 		sptr += sublen;
 	    }
+            if (used_zalloc && subst) {
+                zfree(subst, sublen);
+            }
 	    if (restlen)
 		memcpy(sptr, rest, restlen);
 	    sptr[restlen] = '\0';