[PATCH] Fix a bunch of Coverity-reported defects

[PATCH] Fix a bunch of Coverity-reported defects

[Bart Schaefer]

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

I triaged about 85 defects in the Coverity scan UI.  The majority of
them were spurious, and I marked them "Ignore".  There were 14 that I
felt worthy of small fixes; those are included in the patch below.  I
believe that leaves 14 others where I wasn't confident of a fix;
several  of them are in zftp, as I recall.

A batch of the warnings that I ignored were assignments of one field
of a union to another field of the same union, e.g., a casted long
onto a double, etc., which elicited "overlapping copy" warnings.  I'm
fairly confident we'd have seen things crashing by now if this wasn't
safe, but I mention it in case someone knows why it might be a
problem.

One of those I did NOT fix is this, mentioned recently:

> > *** CID 1547827:  Null pointer dereferences  (FORWARD_NULL)
> > /Src/Modules/pcre.c: 370 in bin_pcre_match()
> > >>>     Passing null pointer "named" to "zpcre_get_substrings", which dereferences it.
>
> This is from Oliver's 51738 (PCRE's alternative DFA), I'm not going to
> interpret futher.

Let me know if there's anything controversial here.

[-- Attachment #2: coverity-defects.txt --]
[-- Type: text/plain, Size: 6116 bytes --]

index 8a7d0a4c5..8b863d5c8 100644
--- a/Src/Modules/zutil.c
+++ b/Src/Modules/zutil.c
@@ -1378,11 +1378,11 @@ rmatch(RParseResult *sm, char *subj, char *var1, char *var2, int comp)
 					     "zregexparse-guard"), !lastval))) {
 		LinkNode aln;
 		char **mend;
-		int len;
+		int len = 0;
 
 		queue_signals();
-		mend = getaparam("mend");
-		len = atoi(mend[0]);
+		if ((mend = getaparam("mend")))
+		    len = atoi(mend[0]);
 		unqueue_signals();
 
 		for (i = len; i; i--)
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 77fce66e8..9b87cad93 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -2249,8 +2249,9 @@ addmatches(Cadata dat, char **argv)
 	    llpl = strlen(lpre);
 	    llsl = strlen(lsuf);
 
-	    if (llpl + (int)strlen(compqiprefix) + (int)strlen(lipre) != origlpre
-	     || llsl + (int)strlen(compqisuffix) + (int)strlen(lisuf) != origlsuf)
+	    /* This used to reference compqiprefix and compqisuffix, why? */
+	    if (llpl + (int)strlen(qipre) + (int)strlen(lipre) != origlpre
+	     || llsl + (int)strlen(qisuf) + (int)strlen(lisuf) != origlsuf)
 		lenchanged = 1;
 
 	    /* Test if there is an existing -P prefix. */
diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index 57789c0f3..cd8c7dd64 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -897,7 +897,7 @@ void
 do_allmatches(UNUSED(int end))
 {
     int first = 1, nm = nmatches - 1, omc = menucmp, oma = menuacc, e;
-    Cmatch *mc;
+    Cmatch *mc = 0;
     struct menuinfo mi;
     char *p = (brbeg ? ztrdup(lastbrbeg->str) : NULL);
 
@@ -915,10 +915,10 @@ do_allmatches(UNUSED(int end))
 #endif
     }
 
+    if (minfo.group)
+	mc = (minfo.group)->matches;
 
-    mc = (minfo.group)->matches;
-
-    while (1) {
+    while (mc) {
 	if (!((*mc)->flags & CMF_ALL)) {
 	    if (!first)
 		accept_last();
@@ -1731,8 +1731,6 @@ calclist(int showall)
                                  width < zterm_columns && nth < g->dcount;
                                  nth++, tcol++) {
 
-                                m = *p;
-
                                 if (tcol == tcols) {
                                     tcol = 0;
                                     tlines++;
@@ -1994,7 +1992,6 @@ printlist(int over, CLPrintFunc printm, int showall)
 		     (listdat.onlyexpl & ((*e)->always > 0 ? 2 : 1)))) {
 		    if (pnl) {
 			putc('\n', shout);
-			pnl = 0;
 			ml++;
 			if (cl >= 0 && --cl <= 1) {
 			    cl = -1;
@@ -2087,7 +2084,6 @@ printlist(int over, CLPrintFunc printm, int showall)
                         (showall || !(m->flags & (CMF_HIDE|CMF_NOLIST)))) {
 			if (pnl) {
 			    putc('\n', shout);
-			    pnl = 0;
 			    ml++;
 			    if (cl >= 0 && --cl <= 1) {
 				cl = -1;
diff --git a/Src/builtin.c b/Src/builtin.c
index 31af66c7c..9e08a1dbc 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6508,6 +6508,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 
     if (OPT_ISSET(ops,'s') && SHTTY == readfd) {
 	struct ttyinfo ti;
+	memset(&ti, 0, sizeof(struct ttyinfo));
 	gettyinfo(&ti);
 	saveti = ti;
 	resettty = 1;
@@ -6606,7 +6607,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 		else if (resettty && SHTTY != -1)
 		    settyinfo(&saveti);
 		if (haso) {
-		    fclose(shout);
+		    if (shout)
+			fclose(shout);
 		    shout = oshout;
 		    SHTTY = -1;
 		}
diff --git a/Src/glob.c b/Src/glob.c
index 63f8a5fa7..bd199ace3 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -1317,7 +1317,8 @@ zglob(LinkList list, LinkNode np, int nountok)
 		sense = 0;
 		if (qualct) {
 		    qn = (struct qual *)hcalloc(sizeof *qn);
-		    qo->or = qn;
+		    if (qo)
+			qo->or = qn;
 		    qo = qn;
 		    qualorct++;
 		    qualct = 0;
diff --git a/Src/hist.c b/Src/hist.c
index bfbcd6ede..448dfddbc 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1359,7 +1359,8 @@ putoldhistentryontop(short keep_going)
 	do {
 	    if (max_unique_ct-- <= 0 || he == hist_ring) {
 		max_unique_ct = 0;
-		he = hist_ring->down;
+		if (hist_ring)
+		    he = hist_ring->down;
 		next = hist_ring;
 		break;
 	    }
@@ -1367,12 +1368,16 @@ putoldhistentryontop(short keep_going)
 	    next = he->down;
 	} while (!(he->node.flags & HIST_DUP));
     }
-    if (he != hist_ring->down) {
+    /* Is it really possible for hist_ring to be NULL here? */
+    if (he && (!hist_ring || he != hist_ring->down)) {
 	he->up->down = he->down;
 	he->down->up = he->up;
 	he->up = hist_ring;
-	he->down = hist_ring->down;
-	hist_ring->down = he->down->up = he;
+	if (hist_ring) {
+	    he->down = hist_ring->down;
+	    hist_ring->down = he;
+	}
+	he->down->up = he;
     }
     hist_ring = he;
 }
@@ -1468,7 +1473,7 @@ should_ignore_line(Eprog prog)
 mod_export int
 hend(Eprog prog)
 {
-    int flag, hookret, stack_pos = histsave_stack_pos;
+    int flag, hookret = 0, stack_pos = histsave_stack_pos;
     /*
      * save:
      * 0: don't save
diff --git a/Src/input.c b/Src/input.c
index dd8f2edc7..d8ac2c0e7 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -643,18 +643,6 @@ zstuff(char **out, const char *fn)
     return len;
 }
 
-/**/
-char *
-ztuff(const char *fn)
-{
-    char *buf;
-    off_t len = zstuff(&buf, fn);
-    if (len > 0)
-	return buf;
-    else
-	return NULL;
-}
-
 /* stuff a whole file into the input queue and print it */
 
 /**/
diff --git a/Src/params.c b/Src/params.c
index 957656e3f..9f0cbcd67 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -6326,10 +6326,9 @@ mod_export Param
 upscope(Param pm, int reflevel)
 {
     Param up = pm->old;
-    while (pm && up && up->level >= reflevel) {
+    while (up && up->level >= reflevel) {
 	pm = up;
-	if (up)
-	    up = up->old;
+	up = up->old;
     }
     return pm;
 }
diff --git a/Src/utils.c b/Src/utils.c
index 790625379..0f66984cd 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -7523,8 +7523,8 @@ restoredir(struct dirsav *d)
     else if (d->level < 0)
 	err = -1;
     if (d->dev || d->ino) {
-	stat(".", &sbuf);
-	if (sbuf.st_ino != d->ino || sbuf.st_dev != d->dev)
+	if (stat(".", &sbuf) < 0 ||
+	    sbuf.st_ino != d->ino || sbuf.st_dev != d->dev)
 	    err = -2;
     }
     return err;

Re: [PATCH] Fix a bunch of Coverity-reported defects

[Mikael Magnusson]

On 10/26/23, Bart Schaefer <schaefer@brasslantern.com> wrote:
> I triaged about 85 defects in the Coverity scan UI.  The majority of
> them were spurious, and I marked them "Ignore".

One thing to keep in mind is it sometimes groups several "occurrences"
of the same issue together, you can switch them with a combo box near
the bottom right of the UI. (maybe a bit late to point this out now).
I've sometimes noticed that the first instance is a false positive
while one of the others are not, but that was years ago, maybe they've
improved this.

> There were 14 that I
> felt worthy of small fixes; those are included in the patch below.  I
> believe that leaves 14 others where I wasn't confident of a fix;
> several  of them are in zftp, as I recall.
>
> A batch of the warnings that I ignored were assignments of one field
> of a union to another field of the same union, e.g., a casted long
> onto a double, etc., which elicited "overlapping copy" warnings.  I'm
> fairly confident we'd have seen things crashing by now if this wasn't
> safe, but I mention it in case someone knows why it might be a
> problem.
>
> One of those I did NOT fix is this, mentioned recently:
>
>> > *** CID 1547827:  Null pointer dereferences  (FORWARD_NULL)
>> > /Src/Modules/pcre.c: 370 in bin_pcre_match()
>> > >>>     Passing null pointer "named" to "zpcre_get_substrings", which
>> > >>> dereferences it.
>>
>> This is from Oliver's 51738 (PCRE's alternative DFA), I'm not going to
>> interpret futher.
>
> Let me know if there's anything controversial here.

I uploaded a new build to coverity with this patch applied and it
seems to be happy with it.

-- 
Mikael Magnusson

Re: [PATCH] Fix a bunch of Coverity-reported defects

[Peter Stephenson]

> On 26/10/2023 04:36 Bart Schaefer <schaefer@brasslantern.com> wrote:
> A batch of the warnings that I ignored were assignments of one field
> of a union to another field of the same union, e.g., a casted long
> onto a double, etc., which elicited "overlapping copy" warnings.  I'm
> fairly confident we'd have seen things crashing by now if this wasn't
> safe, but I mention it in case someone knows why it might be a
> problem.

I guess it's worried about cases where the reads are not atomic
and you might be writing back piecemeal.  I can't believe this is going
to be an issue for 32-bit values, so if there was ever going to be a
problem it would be when there is a read of e.g. a long long or a double
on a 32-bit system.  I rather suspect compiler writers are alive to this
sort of thing anyway.

pws

Re: [PATCH] Fix a bunch of Coverity-reported defects

[Roman Perepelitsa]

On Thu, Oct 26, 2023 at 5:37 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> A batch of the warnings that I ignored were assignments of one field
> of a union to another field of the same union, e.g., a casted long
> onto a double, etc., which elicited "overlapping copy" warnings.  I'm
> fairly confident we'd have seen things crashing by now if this wasn't
> safe, but I mention it in case someone knows why it might be a
> problem.

This can indeed cause problems. The conditions under which it happens
are subtle. Here's an example: https://godbolt.org/z/EvxTzM1hn.

    inline int foo(int* x, float* y) {
        *x = 1;
        *y = 2;
        return *x;
    }

    // Returns either 1 or 0x40000000 depending on the
    // absence or presence of -fno-strict-aliasing.
    int bar() {
        union {
            int x;
            float y;
        } z;
        return foo(&z.x, &z.y);
    }

    // The same as bar() but with the call to foo()
    // manually inlined. Return 0x40000000 with and
    // without -fno-strict-aliasing.
    int baz() {
        union {
            int x;
            float y;
        } z;
        // The following code is equivalent to
        // return foo(&z.x, &z.y).
        int* x = &z.x;
        float* y = &z.y;
        *x = 1;
        *y = 2;
        return *x;
    }

When compiled with `gcc -std=c99 -O2`:

  bar:
          mov     eax, 1
          ret
  baz:
          mov     eax, 0x40000000
          ret

When compiled with `gcc -std=c99 -O2 -fno-strict-aliasing`:

  bar:
          mov     eax, 0x40000000
          ret
  baz:
          mov     eax, 0x40000000
          ret

A simple workaround is to compile with -fno-strict-aliasing. This can
result in slower code but I don't think it's likely to be noticable.

Roman.

Re: [PATCH] Fix a bunch of Coverity-reported defects

[Bart Schaefer]

On Thu, Oct 26, 2023 at 2:19 AM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 5:37 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > A batch of the warnings that I ignored were assignments of one field
> > of a union to another field of the same union, e.g., a casted long
> > onto a double, etc., which elicited "overlapping copy" warnings.
>
> This can indeed cause problems. The conditions under which it happens
> are subtle. Here's an example: https://godbolt.org/z/EvxTzM1hn.
>
>     inline int foo(int* x, float* y) {
>         *x = 1;
>         *y = 2;
>         return *x;
>     }

None of the instances flagged by coverity involved indirection through
pointers, so I suspect we're OK there.

Re: [PATCH] Fix a bunch of Coverity-reported defects

[Bart Schaefer]

On Thu, Oct 26, 2023 at 1:19 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 10/26/23, Bart Schaefer <schaefer@brasslantern.com> wrote:
> > I triaged about 85 defects in the Coverity scan UI.  The majority of
> > them were spurious, and I marked them "Ignore".
>
> One thing to keep in mind is it sometimes groups several "occurrences"
> of the same issue together, you can switch them with a combo box near
> the bottom right of the UI. (maybe a bit late to point this out now).

As far as I could tell each ID number referenced a single occurrence.
A lot of them were like this one:

> > >>>     CID 1547833:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> > >>>     Using variable "*lineptr" as an index to array "typtab".
> > /Src/hist.c: 3809 in histsplitwords()

That is, using a macro to index the typtab array using a *(char*) and
complaining that the byte pointed to could be a negative number.  In
practice this is just not going to happen (or if it does something
else is already desperately wrong).

Many others were about not checking error status of system calls when
in practice there's no sensible recovery mode, e.g., what would we do
instead if setting the close-on-exec flag on an fd does not work?

> I uploaded a new build to coverity with this patch applied and it
> seems to be happy with it.

Thanks.

RE: Re: [PATCH] Fix a bunch of Coverity-reported defects

[zeurkous]

On Thu, 26 Oct 2023 07:25:27 -0700, Bart Schaefer <schaefer@brasslantern.com> wrote:
> Many others were about not checking error status of system calls when
> in practice there's no sensible recovery mode, e.g., what would we do
> instead if setting the close-on-exec flag on an fd does not work?

Fail hard; at the very least, it should be warn(3)'d about.
        --zeurkous.

-- 
Friggin' Machines!