Coverity issue policy

[Tim Rühsen]

Hi,

I let my projects irregularly being checked by Coverity, most issues are
found within gnulib code.

Is there any interest in patches (even if trivial or even when just to
silence Coverity) ? I just ask because maybe you already do these checks
and set them to 'ignore' / 'false positive'. In this case I would still
see these issues and would waste my time on them.

Regards, Tim

[Bruno Haible]

Hi Tim,

Is there any interest in patches ... would waste my time on them.

It depends.
- We want to hear about real bugs, of course.
- If the warnings can be silenced through -Dlint, we would ask you to just
compile with -Dlint.
- If the warning is in code with some #ifs and fixing the warning would
make the #if logic more complex, I'm not sure we would want to fix it.

If you are not sure how you can contribute, you can just send us the list of
warnings and let us deal with them.

Bruno

[Tim Rühsen]

Post by Bruno Haible
Hi Tim,

Is there any interest in patches ... would waste my time on them.

It depends.
- We want to hear about real bugs, of course.
- If the warnings can be silenced through -Dlint, we would ask you to just
compile with -Dlint.
- If the warning is in code with some #ifs and fixing the warning would
make the #if logic more complex, I'm not sure we would want to fix it.
If you are not sure how you can contribute, you can just send us the list of
warnings and let us deal with them.

I could invite you (or anybody else) to view the detected defects. Just
give me an OK and/or your email address before I do so.

From what I can quickly see, some defects might be quite serious.

Regards, Tim

[Paul Eggert]

I'd be interested in seeing the bugs. Not sure I'd have time to fix them all....

[Tim Rühsen]

Post by Paul Eggert
I'd be interested in seeing the bugs. Not sure I'd have time to fix them all....

I added you two as 'viewers', not sure what exactly means. If you need more
access, let me know.

Regards, Tim

[Bruno Haible]

Post by Tim Rühsen
I could invite you (or anybody else) to view the detected defects. Just
give me an OK and/or your email address before I do so.
From what I can quickly see, some defects might be quite serious.

What I can see (in the limited set of gnulib modules that wget uses):

1) Coverity suggests to use 'memmove' instead of 'memcpy' in a couple
of places where it cannot prove that the source and destination memory
regions don't overlap.

I don't know what could make the coverity warnings disappear, but at least
we can add comments that help us verify that there is no issue. Invariants,
as usual.

2) A false warning at
len >> 31 >> 31 >> 2
because the code is prepared for 128-bit integers but coverity "knows" that
'len' is at most 64 bits wide.

3) malloc/free related warnings in glob.c.

Please review the three attached patches.

[Paul Eggert]

Post by Bruno Haible
Please review the three attached patches.

Thanks for looking into it. Those patches all look good do me.

[Bruno Haible]

Post by Paul Eggert
Thanks for looking into it. Those patches all look good do me.

OK, I've pushed them.

Tim, please let me know when there is a new coverity build we could look at.

Bruno

[Tim Rühsen]

Post by Bruno Haible

Post by Paul Eggert
Thanks for looking into it. Those patches all look good do me.

OK, I've pushed them.
Tim, please let me know when there is a new coverity build we could look at.

It's uploaded now and you both have member permissions now.

Regards, Tim

[Bruno Haible]

Post by Tim Rühsen
It's uploaded now and you both have member permissions now.

Next round. Here's a proposed patch, again for glob.c.

[Tim Rühsen]

Post by Bruno Haible

Post by Tim Rühsen
It's uploaded now and you both have member permissions now.

Next round. Here's a proposed patch, again for glob.c.

Did you receive the scan results from Coverity ? If yes, I don't have to ping
you anymore :-)

Anyways, I would be a good idea if one of the gnulib maintainers signs up at
Coverity and adds the gnulib project. To check more modules than wget2 uses.
I guess, there is a 'make check' or a test run that compiles most of gnulib's
sources !?

If you use a CI, you can also automate the Coverity scanning.

Let me know, if i can help.

Regards, Tim

[Bruno Haible]

Post by Tim Rühsen
Anyways, I would be a good idea if one of the gnulib maintainers signs up at
Coverity and adds the gnulib project. To check more modules than wget2 uses.

Done: It's at https://scan.coverity.com/projects/gnu-gnulib?tab=overview

You are welcome to contribute to this code review (as time permits, of course).

Bruno

[Bruno Haible]

Post by Bruno Haible
Next round. Here's a proposed patch, again for glob.c.

No one reviewed this; I've now applied it anyway.


2017-04-01 Bruno Haible <***@clisp.org>

glob: Fix more memory leaks.
* lib/glob.c (glob): Free allocated memory before returning.
Reported by Coverity via Tim Rühsen.

diff --git a/lib/glob.c b/lib/glob.c
index 9305038..d4fdc17 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -726,6 +726,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
pwtmpbuf = malloc (pwbuflen);
if (pwtmpbuf == NULL)
{
+ if (__glibc_unlikely (malloc_name))
+ free (name);
retval = GLOB_NOSPACE;
goto out;
}
@@ -754,6 +756,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (newp == NULL)
{
free (malloc_pwtmpbuf);
+ if (__glibc_unlikely (malloc_name))
+ free (name);
retval = GLOB_NOSPACE;
goto out;
}
@@ -801,10 +805,10 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
}
if (home_dir == NULL || home_dir[0] == '\0')
{
+ if (__glibc_unlikely (malloc_home_dir))
+ free (home_dir);
if (flags & GLOB_TILDE_CHECK)
{
- if (__glibc_unlikely (malloc_home_dir))
- free (home_dir);
retval = GLOB_NOMATCH;
goto out;
}
@@ -1084,9 +1088,14 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
}
else
{
- pglob->gl_pathv[newcount] = strdup (dirname);
- if (pglob->gl_pathv[newcount] == NULL)
- goto nospace;
+ if (__glibc_unlikely (malloc_dirname))
+ pglob->gl_pathv[newcount] = dirname;
+ else
+ {
+ pglob->gl_pathv[newcount] = strdup (dirname);
+ if (pglob->gl_pathv[newcount] == NULL)
+ goto nospace;
+ }
}
pglob->gl_pathv[++newcount] = NULL;
++pglob->gl_pathc;