Some valgrind patches

Some valgrind patches

[Mikael Magnusson]

These are roughly ordered from most to least sure I am that it's a
complete / correct fix. The last 2 in particular I would like comments
on. I'll commit everything up to patch 11 unless someone objects and
nothing after unless someone says it's the right thing to do.

(This is the first time I send more than one patch out at once with git
send-email so hopefully it works right.)

PATCH 01/17: emulate: Handle aborting from mixed -L/-c correctly

[Mikael Magnusson]

Somehow Coverity found this (Issue 1255797, Failure to restore non-local value).
---
 Src/builtin.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Src/builtin.c b/Src/builtin.c
index ebc0654..2b9c4de 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5171,7 +5171,7 @@ bin_emulate(UNUSED(char *nam), char **argv, Options ops, UNUSED(int func))
     if (cmd) {
 	if (opt_L) {
 	    zwarnnam("emulate", "option -L incompatible with -c");
-	    goto restore;
+	    goto restore2;
 	}
 	*--argv = cmd;	/* on stack, never free()d, see execbuiltin() */
     } else
@@ -5209,6 +5209,7 @@ bin_emulate(UNUSED(char *nam), char **argv, Options ops, UNUSED(int func))
     }
     ret = eval(argv);
     sticky = save_sticky;
+restore2:
     emulation = saveemulation;
     memcpy(opts, saveopts, sizeof(opts));
     restorepatterndisables(savepatterns);
-- 
2.2.0.GIT

PATCH 02/17: Don't crash when writing out history if HOST is unset

[Mikael Magnusson]

Found by Coverity (Issue 1255793).
---
 Src/hist.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Src/hist.c b/Src/hist.c
index a006170..8ff9eff 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2801,7 +2801,8 @@ lockhistfile(char *fn, int keep_trying)
 #ifdef HAVE_LINK
 # ifdef HAVE_SYMLINK
 	sprintf(pidbuf, "/pid-%ld/host-", (long)mypid);
-	lnk = bicat(pidbuf, getsparam("HOST"));
+	lnk = getsparam("HOST");
+	lnk = bicat(pidbuf, lnk ? lnk : "");
 	/* We'll abuse fd as our success flag. */
 	while ((fd = symlink(lnk, lockfile)) < 0) {
 	    if (errno != EEXIST) {
-- 
2.2.0.GIT

PATCH 03/17: computil: Check for NULL before passing to strlen

[Mikael Magnusson]

The rest of this function appears to be very careful about checking these,
then forgets in this one spot. Found by Coverity (Issue 1255805).
---
 Src/Zle/computil.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index b11c39f..a81d1dd 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -4060,7 +4060,8 @@ cfp_test_exact(LinkList names, char **accept, char *skipped)
     if (sl > PATH_MAX2)
 	return NULL;
 
-    suf = dyncat(skipped, rembslash(dyncat(compprefix, compsuffix)));
+    suf = dyncat(skipped, rembslash(dyncat(compprefix ? compprefix : "",
+		                           compsuffix ? compsuffix : "")));
 
     for (node = firstnode(names); node; incnode(node)) {
 	l = strlen(p = (char *) getdata(node));
-- 
2.2.0.GIT

PATCH 04/17: zle: size_t is unsigned, use int instead

[Mikael Magnusson]

The function wctomb returns an int according to my manpage, and we
furthermore check if it is negative, and then return it, and the function
signature is int, so declaring it as an int seems to make more sense.
---
 Src/Zle/zle_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index e361e5e..e4ab97a 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -117,7 +117,7 @@ int
 zlecharasstring(ZLE_CHAR_T inchar, char *buf)
 {
 #ifdef MULTIBYTE_SUPPORT
-    size_t ret;
+    int ret;
     char *ptr;
 
 #ifdef __STDC_ISO_10646__
-- 
2.2.0.GIT

PATCH 05/17: compcore: Fix size argument to zfree

[Mikael Magnusson]

Found by Coverity (Issue 1255852), has no impact unless using
--enable-zsh-mem, and even then it is minimal.
---
 Src/Zle/compcore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index b0c6e06..f505605 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -3492,7 +3492,7 @@ freematch(Cmatch m, int nbeg, int nend)
     if (m->brsl)
 	zfree(m->brsl, nend * sizeof(int));
 
-    zfree(m, sizeof(m));
+    zfree(m, sizeof(*m));
 }
 
 /* This frees the groups of matches. */
-- 
2.2.0.GIT

PATCH 06/17: compctl: Remove pointless check

[Mikael Magnusson]

cc has already been derefed a bunch of times leading up to here. Found
by Coverity (Issue 1255841).
---
 Src/Zle/compctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
index d15c2d1..96ad6a2 100644
--- a/Src/Zle/compctl.c
+++ b/Src/Zle/compctl.c
@@ -1515,7 +1515,7 @@ printcompctl(char *s, Compctl cc, int printflags, int ispat)
 	if (cclist & COMP_LIST)
 	    printf(" --");
     }
-    if (cc && cc->xor) {
+    if (cc->xor) {
 	/* print xor'd (+) completions */
 	printf(" +");
 	if (cc->xor != &cc_default)
-- 
2.2.0.GIT

PATCH 07/17: compresult: Remove unneeded NULL check

[Mikael Magnusson]

The variable is set to  if NULL at the start of the function, and derefed
on the previous line. Found by Coverity (Issue 1255843).
---
 Src/Zle/compresult.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 93438a0..dbef7f8 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -1098,7 +1098,7 @@ do_single(Cmatch m)
 		} else {
 		    p = (char *) zhalloc(strlen(prpre) + strlen(str) +
 				 strlen(psuf) + 3);
-		    sprintf(p, "%s%s%s", ((prpre && *prpre) ?
+		    sprintf(p, "%s%s%s", (*prpre ?
 					  prpre : "./"), str, psuf);
 		}
 		/* And do the stat. */
-- 
2.2.0.GIT

PATCH 08/17: subst: remove dead code

[Mikael Magnusson]

Found by Coverity (Issue 1255810).
---
 Src/subst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/subst.c b/Src/subst.c
index 4100803..5136010 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -1317,7 +1317,7 @@ get_intarg(char **s, int *delmatchp)
     if (ret < 0)
 	ret = -ret;
     *delmatchp = arglen;
-    return ret < 0 ? -ret : ret;
+    return ret;
 }
 
 /* Parsing for the (e) flag. */
-- 
2.2.0.GIT

PATCH 09/17: complist: Fix leak of string in clnicezputs

[Mikael Magnusson]

Found by Coverity (Issue 1255808).
---
 Src/Zle/complist.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index b6e7e30..80e5bf9 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -780,6 +780,7 @@ clnicezputs(int do_colors, char *s, int ml)
 	    /* Is the screen full? */
 	    if (ml == mlend - 1 && col == zterm_columns - 1) {
 		mlprinted = ml - oml;
+		free(ums);
 		return 0;
 	    }
 	    if (t < wptr) {
@@ -804,6 +805,7 @@ clnicezputs(int do_colors, char *s, int ml)
 		ml++;
 		if (mscroll && !--mrestlines && (ask = asklistscroll(ml))) {
 		    mlprinted = ml - oml;
+		    free(ums);
 		    return ask;
 		}
 		col -= zterm_columns;
-- 
2.2.0.GIT

PATCH 10/17: whence: use dupstring to not leak memory

[Mikael Magnusson]

All other assignments to buf use the heap, and it's never freed. Found
by Coverity (Issue 1255786).
---
 Src/builtin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/builtin.c b/Src/builtin.c
index 2b9c4de..ae7f53b 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3322,7 +3322,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	    for (pp = path; *pp; pp++) {
 		if (**pp) {
 		    buf = zhtricat(*pp, "/", *argv);
-		} else buf = ztrdup(*argv);
+		} else buf = dupstring(*argv);
 
 		if (iscom(buf)) {
 		    if (wd) {
-- 
2.2.0.GIT

PATCH 11/17: hist: use zhtricat instead of tricat

[Mikael Magnusson]

Found by Coverity (Issue 1255769).
---
 Src/hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/hist.c b/Src/hist.c
index 8ff9eff..d8192c1 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -3111,7 +3111,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
 		     * double quotes.  Whitespace in the middle is
 		     * similarly retained, so just add the parentheses back.
 		     */
-		    p = tricat("((", tokstr, "))");
+		    p = zhtricat("((", tokstr, "))");
 		}
 		break;
 
-- 
2.2.0.GIT

PATCH 12/17: typeset: fix leak of oldval

[Mikael Magnusson]

Found by Coverity (Issue 1255803).
---
 Src/builtin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Src/builtin.c b/Src/builtin.c
index ae7f53b..38d34ae 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2510,6 +2510,8 @@ bin_typeset(char *name, char **argv, Options ops, int func)
 							  asg->name),
 				 func, (on | PM_ARRAY) & ~PM_EXPORTED,
 				 off, roff, asg->value, NULL, ops, 0))) {
+	    if (oldval)
+		zsfree(oldval);
 	    unqueue_signals();
 	    return 1;
 	}
-- 
2.2.0.GIT

PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Mikael Magnusson]

Found by Coverity (Issue 439076).
---
 Src/exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Src/exec.c b/Src/exec.c
index 6b93008..207e8c1 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
 	    if (htok && args) {
 		execsubst(args);
 		if (errflag) {
+		    zsfree(shf->filename);
+		    zfree(shf, sizeof(*shf));
 		    state->pc = end;
 		    return 1;
 		}
-- 
2.2.0.GIT

PATCH 14/17: getsubsargs: free ptr1 before returning

[Mikael Magnusson]

Found by Coverity (Issue 439073).
---
 Src/hist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Src/hist.c b/Src/hist.c
index d8192c1..e65d78b 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -367,6 +367,7 @@ getsubsargs(char *subline, int *gbalp, int *cflagp)
 	zsfree(hsubl);
 	hsubl = ptr1;
     } else if (!hsubl) {		/* fail silently on this */
+	zsfree(ptr1);
 	zsfree(ptr2);
 	return 0;
     }
-- 
2.2.0.GIT

PATCH 15/17: Don't leak ifs stuff

[Mikael Magnusson]

Found by Coverity (Issue 1255785).
---
 Src/utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Src/utils.c b/Src/utils.c
index 390f513..72a0c9c 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -3543,7 +3543,7 @@ inittyptab(void)
     for (t0 = (int)STOUC(Snull); t0 <= (int)STOUC(Nularg); t0++)
 	typtab[t0] |= ITOK | IMETA | INULL;
     for (s = ifs ? ifs : EMULATION(EMULATE_KSH|EMULATE_SH) ?
-	ztrdup(DEFAULT_IFS_SH) : ztrdup(DEFAULT_IFS); *s; s++) {
+	DEFAULT_IFS_SH : DEFAULT_IFS; *s; s++) {
 	int c = STOUC(*s == Meta ? *++s ^ 32 : *s);
 #ifdef MULTIBYTE_SUPPORT
 	if (!isascii(c)) {
@@ -3578,7 +3578,7 @@ inittyptab(void)
 #ifdef MULTIBYTE_SUPPORT
     set_widearray(wordchars, &wordchars_wide);
     set_widearray(ifs ? ifs : EMULATION(EMULATE_KSH|EMULATE_SH) ?
-	ztrdup(DEFAULT_IFS_SH) : ztrdup(DEFAULT_IFS), &ifs_wide);
+	DEFAULT_IFS_SH : DEFAULT_IFS, &ifs_wide);
 #endif
     for (s = SPECCHARS; *s; s++)
 	typtab[STOUC(*s)] |= ISPECIAL;
-- 
2.2.0.GIT

PATCH 16/17: compctl: Comparing array with 0 does nothing, check contents (should it be dropped instead?)

[Mikael Magnusson]

Found by Coverity (Issue 1255780).
---
 Src/Zle/compctl.c | 2 +-
 Src/jobs.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
index 96ad6a2..2a80e6c 100644
--- a/Src/Zle/compctl.c
+++ b/Src/Zle/compctl.c
@@ -3685,7 +3685,7 @@ makecomplistflags(Compctl cc, char *s, int incmd, int compadd)
 
 	for (i = 0; i <= maxjob; i++)
 	    if ((jobtab[i].stat & STAT_INUSE) &&
-		jobtab[i].procs && jobtab[i].procs->text) {
+		jobtab[i].procs && jobtab[i].procs->text[0]) {
 		int stopped = jobtab[i].stat & STAT_STOPPED;
 
 		j = dupstring(jobtab[i].procs->text);
diff --git a/Src/jobs.c b/Src/jobs.c
index c6e1bce..295f4c9 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -2718,7 +2718,7 @@ findjobnam(const char *s)
     for (jobnum = maxjob; jobnum >= 0; jobnum--)
 	if (!(jobtab[jobnum].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
 	    jobtab[jobnum].stat && jobtab[jobnum].procs && jobnum != thisjob &&
-	    jobtab[jobnum].procs->text && strpfx(s, jobtab[jobnum].procs->text))
+	    jobtab[jobnum].procs->text[0] && strpfx(s, jobtab[jobnum].procs->text))
 	    return jobnum;
     return -1;
 }
-- 
2.2.0.GIT

PATCH 17/17: check widthp before deref (is this okay, or should it be *swidep = buf + *widthp ? *widthp : 0; or something)

[Mikael Magnusson]

---
 Src/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/utils.c b/Src/utils.c
index 72a0c9c..c10fb11 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -565,7 +565,7 @@ wcs_nicechar(wchar_t c, size_t *widthp, char **swidep)
 	      *swidep = buf + strlen(buf);
 	    return buf;
 	}
-	if (swidep)
+	if (swidep && widthp)
 	    *swidep = buf + *widthp;
 	return buf;
     }
-- 
2.2.0.GIT

Re: PATCH 17/17: check widthp before deref (is this okay, or should it be *swidep = buf + *widthp ? *widthp : 0; or something)

[Bart Schaefer]

On Jan 6,  6:25am, Mikael Magnusson wrote:
}
} +	if (swidep && widthp)
}  	    *swidep = buf + *widthp;

I think your parenthetical in the subject is on the right track, and
it should be

	if (swidep)
	    *swidep = widthp ? buf + *widthp : buf;

Re: PATCH 06/17: compctl: Remove pointless check

[Bart Schaefer]

On Jan 6,  6:25am, Mikael Magnusson wrote:
} Subject: PATCH 06/17: compctl: Remove pointless check
}
} cc has already been derefed a bunch of times leading up to here. Found
} by Coverity (Issue 1255841).
} -    if (cc && cc->xor) {
} +    if (cc->xor) {

I'm curious, why bother to "fix" this (and a couple of similar others in
later patches in this series)?  It's not *wrong*, and the change is not
a significant optimization.

Re: PATCH 06/17: compctl: Remove pointless check

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 8:53 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 6,  6:25am, Mikael Magnusson wrote:
> } Subject: PATCH 06/17: compctl: Remove pointless check
> }
> } cc has already been derefed a bunch of times leading up to here. Found
> } by Coverity (Issue 1255841).
> } -    if (cc && cc->xor) {
> } +    if (cc->xor) {
>
> I'm curious, why bother to "fix" this (and a couple of similar others in
> later patches in this series)?  It's not *wrong*, and the change is not
> a significant optimization.

It's similar effort to just fix the code or to mark it as a false
positive in coverity, and fixing it makes the code less confusing to
look at as well, so I went with the patches.

-- 
Mikael Magnusson

Re: PATCH 06/17: compctl: Remove pointless check

[Kamil Dudka]

On Monday 05 January 2015 23:53:02 Bart Schaefer wrote:
> On Jan 6,  6:25am, Mikael Magnusson wrote:
> } Subject: PATCH 06/17: compctl: Remove pointless check
> }
> } cc has already been derefed a bunch of times leading up to here. Found
> } by Coverity (Issue 1255841).
> } -    if (cc && cc->xor) {
> } +    if (cc->xor) {
> 
> I'm curious, why bother to "fix" this (and a couple of similar others in
> later patches in this series)?  It's not *wrong*, and the change is not
> a significant optimization.

True, modern compilers would optimize out such checks anyway in some cases.  
Nevertheless, I believe that putting it explicit makes the code easier to 
understand for a human reader.

Kamil

Re: PATCH 12/17: typeset: fix leak of oldval

[Peter Stephenson]

On Tue, 6 Jan 2015 06:25:44 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Found by Coverity (Issue 1255803).
> ---
>  Src/builtin.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Src/builtin.c b/Src/builtin.c
> index ae7f53b..38d34ae 100644
> --- a/Src/builtin.c
> +++ b/Src/builtin.c
> @@ -2510,6 +2510,8 @@ bin_typeset(char *name, char **argv, Options ops, int func)
>  							  asg->name),
>  				 func, (on | PM_ARRAY) & ~PM_EXPORTED,
>  				 off, roff, asg->value, NULL, ops, 0))) {
> +	    if (oldval)
> +		zsfree(oldval);
>  	    unqueue_signals();
>  	    return 1;
>  	}

That's certainly logical --- couldn't be worse than harmless even if
there's something very strange going on.

pws

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Peter Stephenson]

On Tue, 6 Jan 2015 06:25:45 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Found by Coverity (Issue 439076).
> ---
>  Src/exec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Src/exec.c b/Src/exec.c
> index 6b93008..207e8c1 100644
> --- a/Src/exec.c
> +++ b/Src/exec.c
> @@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
>  	    if (htok && args) {
>  		execsubst(args);
>  		if (errflag) {
> +		    zsfree(shf->filename);
> +		    zfree(shf, sizeof(*shf));
>  		    state->pc = end;
>  		    return 1;
>  		}

Can't see how that can be wrong.  Nothing else can take owernship of
shf in the error case.

pws

Re: PATCH 14/17: getsubsargs: free ptr1 before returning

[Peter Stephenson]

On Tue, 6 Jan 2015 06:25:46 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Found by Coverity (Issue 439073).
> ---
>  Src/hist.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Src/hist.c b/Src/hist.c
> index d8192c1..e65d78b 100644
> --- a/Src/hist.c
> +++ b/Src/hist.c
> @@ -367,6 +367,7 @@ getsubsargs(char *subline, int *gbalp, int *cflagp)
>  	zsfree(hsubl);
>  	hsubl = ptr1;
>      } else if (!hsubl) {		/* fail silently on this */
> +	zsfree(ptr1);
>  	zsfree(ptr2);
>  	return 0;
>      }

Looks fine.

Separately, it's a little odd we test !ptr1 a few lines above but not
!ptr2.  I don't see any sign that hsubr = NULL is valid.  We might want

    if (!ptr2) {
        zsfree(ptr1);
	return 1;
    }

However, it looks like that can only be a memory failure from
hdynread2() that the function makes no attempt to handle internally.
There are a zillion other places where we have no idea how to fail
gracefully even with much larger chunks of memory, so the existing test
for !ptr1 doesn't really make sense on its own.

pws

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 10:54 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 6 Jan 2015 06:25:45 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> Found by Coverity (Issue 439076).
>> ---
>>  Src/exec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Src/exec.c b/Src/exec.c
>> index 6b93008..207e8c1 100644
>> --- a/Src/exec.c
>> +++ b/Src/exec.c
>> @@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
>>           if (htok && args) {
>>               execsubst(args);
>>               if (errflag) {
>> +                 zsfree(shf->filename);
>> +                 zfree(shf, sizeof(*shf));
>>                   state->pc = end;
>>                   return 1;
>>               }
>
> Can't see how that can be wrong.  Nothing else can take owernship of
> shf in the error case.

I think what I was unsure of in this one, but forgot to write, is if
freeeprog(shf->funcdef) should also be called here? It looks like for
!names, it's all allocated on the heap, but then the non-error path
does free it explicitly anyway (and it looked like freeeprog just
doesn't touch it if it was on the heap), is that just paranoia? And
likewise for shf->redir. And since we have all these heap vs malloc
checks already, why not use zhalloc for shf too? Although the code
might end up neater if we just always do the malloc + free regardless
since there's a bunch of almost-identical code for that stuff.

-- 
Mikael Magnusson

Re: PATCH 15/17: Don't leak ifs stuff

[Peter Stephenson]

On Tue, 6 Jan 2015 06:25:47 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Found by Coverity (Issue 1255785).
> ---
>  Src/utils.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Src/utils.c b/Src/utils.c
> index 390f513..72a0c9c 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -3543,7 +3543,7 @@ inittyptab(void)
>      for (t0 = (int)STOUC(Snull); t0 <= (int)STOUC(Nularg); t0++)
>  	typtab[t0] |= ITOK | IMETA | INULL;
>      for (s = ifs ? ifs : EMULATION(EMULATE_KSH|EMULATE_SH) ?
> -	ztrdup(DEFAULT_IFS_SH) : ztrdup(DEFAULT_IFS); *s; s++) {
> +	DEFAULT_IFS_SH : DEFAULT_IFS; *s; s++) {
>  	int c = STOUC(*s == Meta ? *++s ^ 32 : *s);
>  #ifdef MULTIBYTE_SUPPORT
>  	if (!isascii(c)) {
> @@ -3578,7 +3578,7 @@ inittyptab(void)
>  #ifdef MULTIBYTE_SUPPORT
>      set_widearray(wordchars, &wordchars_wide);
>      set_widearray(ifs ? ifs : EMULATION(EMULATE_KSH|EMULATE_SH) ?
> -	ztrdup(DEFAULT_IFS_SH) : ztrdup(DEFAULT_IFS), &ifs_wide);
> +	DEFAULT_IFS_SH : DEFAULT_IFS, &ifs_wide);
>  #endif
>      for (s = SPECCHARS; *s; s++)
>  	typtab[STOUC(*s)] |= ISPECIAL;

That looks OK.

ifs gets initialised in that way during setupvals().  I guess the code
in inittyptab() just got copied from there when someone spotted it could
happen before setupvals() ran, and the code didn't get properly edited
when copied.

pws

Re: PATCH 16/17: compctl: Comparing array with 0 does nothing, check contents (should it be dropped instead?)

[Peter Stephenson]

On Tue, 6 Jan 2015 06:25:48 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> Found by Coverity (Issue 1255780).
> ---
>  Src/Zle/compctl.c | 2 +-
>  Src/jobs.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
> index 96ad6a2..2a80e6c 100644
> --- a/Src/Zle/compctl.c
> +++ b/Src/Zle/compctl.c
> @@ -3685,7 +3685,7 @@ makecomplistflags(Compctl cc, char *s, int incmd, int compadd)
>  
>  	for (i = 0; i <= maxjob; i++)
>  	    if ((jobtab[i].stat & STAT_INUSE) &&
> -		jobtab[i].procs && jobtab[i].procs->text) {
> +		jobtab[i].procs && jobtab[i].procs->text[0]) {
>  		int stopped = jobtab[i].stat & STAT_STOPPED;
>  
>  		j = dupstring(jobtab[i].procs->text);
> diff --git a/Src/jobs.c b/Src/jobs.c
> index c6e1bce..295f4c9 100644
> --- a/Src/jobs.c
> +++ b/Src/jobs.c
> @@ -2718,7 +2718,7 @@ findjobnam(const char *s)
>      for (jobnum = maxjob; jobnum >= 0; jobnum--)
>  	if (!(jobtab[jobnum].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
>  	    jobtab[jobnum].stat && jobtab[jobnum].procs && jobnum != thisjob &&
> -	    jobtab[jobnum].procs->text && strpfx(s, jobtab[jobnum].procs->text))
> +	    jobtab[jobnum].procs->text[0] && strpfx(s, jobtab[jobnum].procs->text))
>  	    return jobnum;
>      return -1;
>  }

This is because text is an array within the structure.  I haven't looked
to see if we always sanitise the maximum length (including NULL) to
JOBTEXTSIZE; I don't know if Coverity would know about that.  We
initialise it as

    if (text)
	strcpy(pn->text, text);
    else
	*pn->text = '\0';

So the change is fine but in the second case the remaining test might
not be needed:  the strpfx() will fail unless s is also empty and if it is
also empty logically it should probably succeed (how useful that is is
another question).  But probably not worth worrying about.

pws

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Peter Stephenson]

On Tue, 6 Jan 2015 11:14:12 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> >> @@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
> >>           if (htok && args) {
> >>               execsubst(args);
> >>               if (errflag) {
> >> +                 zsfree(shf->filename);
> >> +                 zfree(shf, sizeof(*shf));
> >>                   state->pc = end;
> >>                   return 1;
> >>               }
> >
> > Can't see how that can be wrong.  Nothing else can take owernship of
> > shf in the error case.
> 
> I think what I was unsure of in this one, but forgot to write, is if
> freeeprog(shf->funcdef) should also be called here? It looks like for
> !names, it's all allocated on the heap, but then the non-error path
> does free it explicitly anyway (and it looked like freeeprog just
> doesn't touch it if it was on the heap), is that just paranoia?

It should certainly be consistently one thing or the other, yes, so
probably better add it, and if so I suppose the paranoid test for redir
as well.

There's no zsfree(shf->filename) after the settrap() failure in the
other branch.  I can't see how that can be anything other than a leak
--- nothing special happens to shf->filename before shf is freed.

> And since we have all these heap vs malloc checks already, why not use
> zhalloc for shf too?  Although the code might end up neater if we just
> always do the malloc + free regardless since there's a bunch of
> almost-identical code for that stuff.

The permanent alloc appears to be needed in the case of names != NULL to
add the function to the main table --- but the current hotch potch might
certainly be improved by doing one thing or the other:  always
permanently allocate everything in one case and always use heap
allocation in the other; or alternatively always malloc and if necessary
free.  That needs more thought.

pws

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 11:34 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 6 Jan 2015 11:14:12 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> >> @@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
>> >>           if (htok && args) {
>> >>               execsubst(args);
>> >>               if (errflag) {
>> >> +                 zsfree(shf->filename);
>> >> +                 zfree(shf, sizeof(*shf));
>> >>                   state->pc = end;
>> >>                   return 1;
>> >>               }
>> >
>> > Can't see how that can be wrong.  Nothing else can take owernship of
>> > shf in the error case.
>>
>> I think what I was unsure of in this one, but forgot to write, is if
>> freeeprog(shf->funcdef) should also be called here? It looks like for
>> !names, it's all allocated on the heap, but then the non-error path
>> does free it explicitly anyway (and it looked like freeeprog just
>> doesn't touch it if it was on the heap), is that just paranoia?
>
> It should certainly be consistently one thing or the other, yes, so
> probably better add it, and if so I suppose the paranoid test for redir
> as well.
>
> There's no zsfree(shf->filename) after the settrap() failure in the
> other branch.  I can't see how that can be anything other than a leak
> --- nothing special happens to shf->filename before shf is freed.

Okay, I've added the freeeprogs to the path in the above patch, and
the zsfree of filename in the trap path.

>> And since we have all these heap vs malloc checks already, why not use
>> zhalloc for shf too?  Although the code might end up neater if we just
>> always do the malloc + free regardless since there's a bunch of
>> almost-identical code for that stuff.
>
> The permanent alloc appears to be needed in the case of names != NULL to
> add the function to the main table --- but the current hotch potch might
> certainly be improved by doing one thing or the other:  always
> permanently allocate everything in one case and always use heap
> allocation in the other; or alternatively always malloc and if necessary
> free.  That needs more thought.
>
> pws

And I suppose I'll leave this for an even colder winter day. Or we
could point potential new contributors looking for areas to help to
this code, as an introductory exercise to set their expectations
right.

-- 
Mikael Magnusson

Re: PATCH 06/17: compctl: Remove pointless check

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 10:43 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Monday 05 January 2015 23:53:02 Bart Schaefer wrote:
>> On Jan 6,  6:25am, Mikael Magnusson wrote:
>> } Subject: PATCH 06/17: compctl: Remove pointless check
>> }
>> } cc has already been derefed a bunch of times leading up to here. Found
>> } by Coverity (Issue 1255841).
>> } -    if (cc && cc->xor) {
>> } +    if (cc->xor) {
>>
>> I'm curious, why bother to "fix" this (and a couple of similar others in
>> later patches in this series)?  It's not *wrong*, and the change is not
>> a significant optimization.
>
> True, modern compilers would optimize out such checks anyway in some cases.
> Nevertheless, I believe that putting it explicit makes the code easier to
> understand for a human reader.

If you look at the bigger context, it's something like
if (cc->foo) {
...
}
if (cc->bar) {
...
}
if (cc->baz) {
...
}
repeated maybe 17 times, and then suddenly
if (cc && cc->xor) {
...
}
and it sort of makes you wonder if something suddenly could have
changed cc, but indeed there is not.

-- 
Mikael Magnusson

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Peter Stephenson]

On Tue, 6 Jan 2015 12:06:32 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> And I suppose I'll leave this for an even colder winter day. Or we
> could point potential new contributors looking for areas to help to
> this code, as an introductory exercise to set their expectations
> right.

There's an essay in frustration waiting to come out of that...

Maybe we could *not* point potential new contributors at the current
code and they could just do it right this time...?  Maybe in D?

pws

Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 12:18 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 6 Jan 2015 12:06:32 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> And I suppose I'll leave this for an even colder winter day. Or we
>> could point potential new contributors looking for areas to help to
>> this code, as an introductory exercise to set their expectations
>> right.
>
> There's an essay in frustration waiting to come out of that...
>
> Maybe we could *not* point potential new contributors at the current
> code and they could just do it right this time...?  Maybe in D?

Haha, yes, that might be a better idea. (I hope it was somewhat
obvious that I was kidding too, I'm not quite _that_ evil).

-- 
Mikael Magnusson

Re: PATCH 17/17: check widthp before deref (is this okay, or should it be *swidep = buf + *widthp ? *widthp : 0; or something)

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 8:49 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 6,  6:25am, Mikael Magnusson wrote:
> }
> } +     if (swidep && widthp)
> }           *swidep = buf + *widthp;
>
> I think your parenthetical in the subject is on the right track, and
> it should be
>
>         if (swidep)
>             *swidep = widthp ? buf + *widthp : buf;

Okay, I've updated the patch in my queue to this version. And with
that I think all the feedback has been handled, I won't resend the
whole series before committing. If anyone wants to get some
last-minute reviewing in, you can snoop on my local tree at
http://comm.it.cx/cgit/zsh-cvs/log/

-- 
Mikael Magnusson

Re: PATCH 06/17: compctl: Remove pointless check

[Kamil Dudka]

On Tuesday 06 January 2015 12:09:33 Mikael Magnusson wrote:
> On Tue, Jan 6, 2015 at 10:43 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> > On Monday 05 January 2015 23:53:02 Bart Schaefer wrote:
> >> On Jan 6,  6:25am, Mikael Magnusson wrote:
> >> } Subject: PATCH 06/17: compctl: Remove pointless check
> >> }
> >> } cc has already been derefed a bunch of times leading up to here. Found
> >> } by Coverity (Issue 1255841).
> >> } -    if (cc && cc->xor) {
> >> } +    if (cc->xor) {
> >> 
> >> I'm curious, why bother to "fix" this (and a couple of similar others in
> >> later patches in this series)?  It's not *wrong*, and the change is not
> >> a significant optimization.
> > 
> > True, modern compilers would optimize out such checks anyway in some
> > cases.
> > Nevertheless, I believe that putting it explicit makes the code easier to
> > understand for a human reader.
> 
> If you look at the bigger context, it's something like
> if (cc->foo) {
> ...
> }
> if (cc->bar) {
> ...
> }
> if (cc->baz) {
> ...
> }
> repeated maybe 17 times, and then suddenly
> if (cc && cc->xor) {
> ...
> }
> and it sort of makes you wonder if something suddenly could have
> changed cc, but indeed there is not.

Sorry for the confusion.  I wanted to say that your patch makes it easier to 
understand for a human reader (by not checking something that is guaranteed
to hold at that point).

Kamil

Re: Some valgrind patches

[Mikael Magnusson]

On Tue, Jan 6, 2015 at 6:25 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> These are roughly ordered from most to least sure I am that it's a
> complete / correct fix. The last 2 in particular I would like comments
> on. I'll commit everything up to patch 11 unless someone objects and
> nothing after unless someone says it's the right thing to do.
>
> (This is the first time I send more than one patch out at once with git
> send-email so hopefully it works right.)

I just now noticed I wrote valgrind in the subject, I meant coverity
obviously...

-- 
Mikael Magnusson

Re: PATCH 06/17: compctl: Remove pointless check

[Ray Andrews]

On 01/06/2015 01:43 AM, Kamil Dudka wrote:
> On Monday 05 January 2015 23:53:02 Bart Schaefer wrote:
>> On Jan 6,  6:25am, Mikael Magnusson wrote:
>> } Subject: PATCH 06/17: compctl: Remove pointless check
>> }
>> } cc has already been derefed a bunch of times leading up to here. Found
>> } by Coverity (Issue 1255841).
>> } -    if (cc && cc->xor) {
>> } +    if (cc->xor) {
>>
>> I'm curious, why bother to "fix" this (and a couple of similar others in
>> later patches in this series)?  It's not *wrong*, and the change is not
>> a significant optimization.
> True, modern compilers would optimize out such checks anyway in some cases.
> Nevertheless, I believe that putting it explicit makes the code easier to
> understand for a human reader.
>
> Kamil
>
Myself, if I saw some code that appeared pointless, I would sit there 
staring at it until I educated myself as to why it was there, because I 
wouldn't expect that pointless code would ever be tolerated.

Re: PATCH 16/17: compctl: Comparing array with 0 does nothing, check contents (should it be dropped instead?)

[Ray Andrews]

On 01/05/2015 09:25 PM, Mikael Magnusson wrote:
> Found by Coverity (Issue 1255780).
This Coverity seems like quite something.  I've been poking around their 
website for half an hour, and tho it seems to be payware, I can't 
discover how much it costs--or is Mikael doing all this via the demo?

Re: PATCH 16/17: compctl: Comparing array with 0 does nothing, check contents (should it be dropped instead?)

[İsmail Dönmez]

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

Hi,

On Tue, Jan 6, 2015 at 6:23 PM, Ray Andrews <rayandrews@eastlink.ca> wrote:

> On 01/05/2015 09:25 PM, Mikael Magnusson wrote:
>
>> Found by Coverity (Issue 1255780).
>>
> This Coverity seems like quite something.  I've been poking around their
> website for half an hour, and tho it seems to be payware, I can't discover
> how much it costs--or is Mikael doing all this via the demo?
>
>
It is free for open source projects. See http://scan.coverity.com

ismail