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

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

[Sebastian Gniazdowski]

Hello,
managed to finish VATS, V..A.. Test Suite. It is integrated with Zsh build system. Error definition looks like this:

errors2+=( "* / zsh_main / setupvals / gettimeofday / *" )

The matching is done by a Zsh script. Rewritten python module "colour-valgrind" to Zsh, output is colored.

Ran for all tests except for V01zmodload.ztst, it produces too rich output for today. At the bottom of this email are reported leaks for other tests. VATS run looks like following:

https://asciinema.org/a/125035

The directory to be just copied to Zsh root, if accepted:

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

Plus 2 simple configure.ac updates (patch in the repo). I must say that as Valgrind tests are adjecent to long duration, much output and a burden, with VATS, before I noticed, I was at 2/3 of the tests. It really helps. On 32-bit Linux I didn't bother to run normal tests.

I tested VATS and zredis at:

Ubuntu 14.04.4 LTS TT, 32 bit
redis-server 2.8.4, jemalloc 3.4.1
- An old machine, GCC 4.8.4, fixed pure-C and limited-/bin/sh problems, all tests and Valgrind tests pass.

OS X 10.11.5, 64 bit
redis-server 3.2.9 malloc=libc
- all tests and valgrind test pass

FreeBSD 10.3-RELEASE-p2 32 bit
redis-server 3.2.8, malloc=libc
- Zgdbm and zredis compiles. All tests pass. Valgrind always reports illegal op code, no tests ran, however VATS works fine.

I also have a general-purpose VATS project: https://github.com/psprint/VATS

============ The maybe-suspicious reports ============

A01:
==39931== 64 (24 direct, 40 indirect) bytes in 1 blocks are definitely lost in loss record 379 of 605
==39931==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==39931==    by 0x100062A05: zalloc (mem.c:966)
==39931==    by 0x10005B553: znewlinklist (linklist.c:120)
==39931==    by 0x1000511F1: addfilelist (jobs.c:1192)
==39931==    by 0x100026EDA: execpline2 (exec.c:1928)
==39931==    by 0x10001FF50: execpline (exec.c:1602)
==39931==    by 0x10001F0DD: execlist (exec.c:1360)
==39931==    by 0x10001EA2B: execode (exec.c:1141)
==39931==    by 0x100015F3E: eval (builtin.c:5809)
==39931==    by 0x10000BE5F: bin_eval (builtin.c:5995)
==39931==    by 0x100001AE9: execbuiltin (builtin.c:485)
==39931==    by 0x10002BC64: execcmd_exec (exec.c:3958)

A04:
==41041== 15 bytes in 1 blocks are definitely lost in loss record 150 of 604
==41041==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==41041==    by 0x100062A05: zalloc (mem.c:966)
==41041==    by 0x10009A270: bicat (string.c:163)
==41041==    by 0x1000B07F4: gettempname (utils.c:2158)
==41041==    by 0x1000227F4: getoutputfile (exec.c:4573)
==41041==    by 0x10009ABE1: stringsubst (subst.c:179)
==41041==    by 0x10009A541: prefork (subst.c:85)
==41041==    by 0x1000282DC: execcmd_exec (exec.c:3026)
==41041==    by 0x100026C88: execpline2 (exec.c:1873)
==41041==    by 0x10001FF50: execpline (exec.c:1602)
==41041==    by 0x10001F0DD: execlist (exec.c:1360)
==41041==    by 0x10001EA2B: execode (exec.c:1141)

A06:
==50133== 4,096 bytes in 1 blocks are definitely lost in loss record 534 of 544
==50133==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==50133==    by 0x100435AF5: __smakebuf (in /usr/lib/system/libsystem_c.dylib)
==50133==    by 0x1004396D0: __srefill0 (in /usr/lib/system/libsystem_c.dylib)
==50133==    by 0x1004397B8: __srefill (in /usr/lib/system/libsystem_c.dylib)
==50133==    by 0x10043988B: __srget (in /usr/lib/system/libsystem_c.dylib)
==50133==    by 0x1004327DD: fgetc (in /usr/lib/system/libsystem_c.dylib)
==50133==    by 0x10004D25E: shingetline (input.c:152)
==50133==    by 0x10004DB47: inputline (input.c:278)
==50133==    by 0x10004D784: ingetc (input.c:226)
==50133==    by 0x100056DE5: gettok (lex.c:611)
==50133==    by 0x100056B58: zshlex (lex.c:275)
==50133==    by 0x10007C516: parse_event (parse.c:569)

B07:
==60311== 32 bytes in 8 blocks are definitely lost in loss record 283 of 574
==60311==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==60311==    by 0x100062A05: zalloc (mem.c:966)
==60311==    by 0x100099F28: ztrdup (string.c:83)
==60311==    by 0x100048DC8: parseopts (init.c:411)
==60311==    by 0x100015968: bin_emulate (builtin.c:5908)
==60311==    by 0x100001AE9: execbuiltin (builtin.c:485)
==60311==    by 0x10002BC64: execcmd_exec (exec.c:3958)
==60311==    by 0x100026C88: execpline2 (exec.c:1873)
==60311==    by 0x10001FF50: execpline (exec.c:1602)
==60311==    by 0x10001F0DD: execlist (exec.c:1360)
==60311==    by 0x10001EA2B: execode (exec.c:1141)
==60311==    by 0x100015F3E: eval (builtin.c:5809)

C01:
==61832== 17 bytes in 1 blocks are definitely lost in loss record 180 of 592
==61832==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==61832==    by 0x100062A05: zalloc (mem.c:966)
==61832==    by 0x100099F28: ztrdup (string.c:83)
==61832==    by 0x1000623C5: lexconstant (math.c:538)
==61832==    by 0x100061093: zzlex (math.c:795)
==61832==    by 0x10005FF62: mathparse (math.c:1522)
==61832==    by 0x10005FBFB: mathevall (math.c:409)
==61832==    by 0x10005F917: matheval (math.c:1427)
==61832==    by 0x10009DB14: arithsubst (subst.c:4059)
==61832==    by 0x10009B12A: stringsubst (subst.c:293)
==61832==    by 0x10009A541: prefork (subst.c:85)
==61832==    by 0x1000282DC: execcmd_exec (exec.c:3026)

C02:
==62797== 104 (24 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 429 of 587
==62797==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==62797==    by 0x100062A05: zalloc (mem.c:966)
==62797==    by 0x10005B553: znewlinklist (linklist.c:120)
==62797==    by 0x1000511F1: addfilelist (jobs.c:1192)
==62797==    by 0x100026EDA: execpline2 (exec.c:1928)
==62797==    by 0x10001FF50: execpline (exec.c:1602)
==62797==    by 0x10001F0DD: execlist (exec.c:1360)
==62797==    by 0x10002662C: execcursh (exec.c:428)
==62797==    by 0x10002B388: execcmd_exec (exec.c:3784)
==62797==    by 0x100026C88: execpline2 (exec.c:1873)
==62797==    by 0x10001FF50: execpline (exec.c:1602)
==62797==    by 0x10001F0DD: execlist (exec.c:1360)

C03:
==63476== 15 bytes in 1 blocks are definitely lost in loss record 147 of 581
==63476==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==63476==    by 0x100062A05: zalloc (mem.c:966)
==63476==    by 0x10009A270: bicat (string.c:163)
==63476==    by 0x1000B07F4: gettempname (utils.c:2158)
==63476==    by 0x1000227F4: getoutputfile (exec.c:4573)
==63476==    by 0x10009ABE1: stringsubst (subst.c:179)
==63476==    by 0x10009A541: prefork (subst.c:85)
==63476==    by 0x1000282DC: execcmd_exec (exec.c:3026)
==63476==    by 0x100026C88: execpline2 (exec.c:1873)
==63476==    by 0x10001FF50: execpline (exec.c:1602)
==63476==    by 0x10001F0DD: execlist (exec.c:1360)
==63476==    by 0x10001EA2B: execode (exec.c:1141)

C04:
==63900== 119 (24 direct, 95 indirect) bytes in 1 blocks are definitely lost in loss record 434 of 582
==63900==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==63900==    by 0x100062A05: zalloc (mem.c:966)
==63900==    by 0x10005B553: znewlinklist (linklist.c:120)
==63900==    by 0x1000511F1: addfilelist (jobs.c:1192)
==63900==    by 0x100022DCC: getproc (exec.c:4736)
==63900==    by 0x10009ABCB: stringsubst (subst.c:177)
==63900==    by 0x10009A541: prefork (subst.c:85)
==63900==    by 0x100021409: execsubst (exec.c:2545)
==63900==    by 0x100025EDD: execfuncdef (exec.c:5038)
==63900==    by 0x10002B239: execcmd_exec (exec.c:3770)
==63900==    by 0x100026C88: execpline2 (exec.c:1873)
==63900==    by 0x10001FF50: execpline (exec.c:1602)

D04:
==66820== 80 bytes in 5 blocks are definitely lost in loss record 436 of 615
==66820==    at 0x10010A681: malloc (in /usr/local/Cellar/valgrind/3.12.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==66820==    by 0x100062A05: zalloc (mem.c:966)
==66820==    by 0x1000B633A: mkarray (utils.c:3865)
==66820==    by 0x1000A1B70: paramsubst (subst.c:3078)
==66820==    by 0x10009AF24: stringsubst (subst.c:247)
==66820==    by 0x10009A541: prefork (subst.c:85)
==66820==    by 0x1000282DC: execcmd_exec (exec.c:3026)
==66820==    by 0x100026C88: execpline2 (exec.c:1873)
==66820==    by 0x10001FF50: execpline (exec.c:1602)
==66820==    by 0x10001F0DD: execlist (exec.c:1360)
==66820==    by 0x10001EA2B: execode (exec.c:1141)
==66820==    by 0x100015F3E: eval (builtin.c:5809)


--
Sebastian Gniazdowski
psprint /at/ zdharma.org

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

[Bart Schaefer]

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'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).

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

[Sebastian Gniazdowski]

[-- 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';

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

[Bart Schaefer]

On Jun 18,  1:18pm, Sebastian Gniazdowski wrote:
}
} 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.

Hmm, looking more carefully at this, the problem actually seems to be that
getoutputfile() doesn't need to pass 0 to gettempname() any longer.  There
used to be a direct call to zaddlinknode() in getoutputfile(), which needed
a zalloc() string, but that was replaced by addfilelist() which does its own
internal ztrdup().

So the fix is a whole lot simpler:

diff --git a/Src/exec.c b/Src/exec.c
index debb0ae..0a96879 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4570,7 +4570,7 @@ getoutputfile(char *cmd, char **eptr)
     }
     if (!(prog = parsecmd(cmd, eptr)))
 	return NULL;
-    if (!(nam = gettempname(NULL, 0)))
+    if (!(nam = gettempname(NULL, 1)))
 	return NULL;
 
     if ((s = simple_redir_name(prog, REDIR_HERESTR))) {
@@ -4601,7 +4601,7 @@ getoutputfile(char *cmd, char **eptr)
 	    suffix = dyncat(nam, unmeta(suffix));
 	    if (link(nam, suffix) == 0) {
 		addfilelist(nam, 0);
-		nam = ztrdup(suffix);
+		nam = suffix;
 	    }
 	}
     }

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

[Sebastian Gniazdowski]

On 18 czerwca 2017 at 18:24:18, Bart Schaefer (schaefer@brasslantern.com) wrote:
> Hmm, looking more carefully at this, the problem actually seems to be that
> getoutputfile() doesn't need to pass 0 to gettempname() any longer. There
> used to be a direct call to zaddlinknode() in getoutputfile(), which needed
> a zalloc() string, but that was replaced by addfilelist() which does its own
> internal ztrdup().
> 
> So the fix is a whole lot simpler:
> 
> diff --git a/Src/exec.c b/Src/exec.c
...

OK, checked that A04 doesn't output errors

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org