Re: tar + cpio - covscan issues

[Kamil Dudka]

On Saturday, April 10, 2021 3:58:57 PM CEST Bruno Haible wrote:
> Kamil Dudka wrote:
> Paul and I receive a mail with the *new* issues once a week. We never review
> the same issue more than once.

I was not talking about the private notifications you get from Coverity Scan.  
I meant the public reports on this mailing-list like the one that Ondrej sent.  
As gnulib is embedded in multiple RPM packages of Fedora/RHEL, such reports
are going to come periodically until you change your attitude to handling 
false positives upstream.

> > So you have access to this UI, not everybody does.  Some developers prefer
> > terminal-based workflow over web-based UI.
> 
> I didn't know a different workflow was possible.

Red Hat uses csdiff to process the results of static analyzers (not only from 
Coverity).  It is an open source tool that uses open data formats:

    https://github.com/kdudka/csdiff

> But in that workflow, in which you control everything yourself (no SaaS),
> you can surely commit into the repo
>   - either the list of issues produced by the last run, or
>   - a list of issues that you have found to be false ones,
> and use that information to limit what you have to review in the next run?

Sure, we use both.  The problem is that this is a duplicated effort to your 
reviews of Coverity results upstream.  And many downstream consumers have to 
duplicate the effort as well.

The main advantage of code improvements and inline code annotations is that 
they travel together with the source code when the files are moved in the 
source tree, across the projects, or embedded into other projects.  All the 
downstream consumers can consume these improvements at no additional cost.

> No one forces you to review the same false positives over and over again.
> 
> > > 2) About 80%
> > > to 90% of the reported issues are false issues. We would be seriously
> > > contorting the source code if we attempted to change the code to avoid
> > > the
> > > reports.
> > 
> > If you keep fixing real issues and ignoring false positives, such a
> > situation is kind of expected.
> 
> So you are in favour of adding workarounds such as the proposed
> 
>       if (copy != NULL)
>         {
>           data = NULL;
>           return copy;
>         }
>       else
>         return data;
> 
> in 5 to 10 places, in order to get a useful warning in 1 place? I am not.
> 
> Bruno

This is a really bad example and I agree that such changes should be rejected.  
It over-complicates the code and introduces a dead store, which will probably 
trigger some noise with other tools.

By the way, this was supposed to eliminate a report of Cppcheck, which the 
mentioned UI of Coverity does not cover at all.  It could be easily silenced 
with an inline suppression though:

--- a/gnu/malloc/scratch_buffer_dupfree.c
+++ b/gnu/malloc/scratch_buffer_dupfree.c
@@ -35,6 +35,7 @@ __libc_scratch_buffer_dupfree (struct scratch_buffer 
*buffer, size_t size)
   else
     {
       void *copy = realloc (data, size);
+      /* cppcheck-suppress memleak */
       return copy != NULL ? copy : data;
     }
 }