Mass filing of clang warnings

[Hans-Christoph Steiner]

Hey Werner,

While I do not thing that mass filing of bugs without warning is a good idea,
I do think you should reconsider the usefulness of static code analyzers like
clang's warnings and cppcheck.

It is true that they are not as intelligent as people, and don't always
understand every valid variant of C code.  But what they do far better than
people is tirelessly run again and again, checking the entire code base on
every commit.  cppcheck did catch real issues that you fixed.  I ran it and
reported it here, and you confirmed some of them and fixed them.  Also, anyone
can use these tools automatically on the any public code base.  So they can
instead use them to tirelessly search for exploits in GnuPG.

To use these tools effectively, that means that the devs have to write code
that the static analyzers understand.  This usually also means that the code
is easier for humans to understand, that is also a big benefit.  It does mean
doing some annoying work sometimes, and fixing little annoyances here and
there, but I think using static analyzers pays off in a big way overall.

.hc

Werner Koch:
> Hi!
> 
> While looking at the bug tracker, I figured that a few days ago one guy
> filed dozens of bugs which in most cases are warnings issued by Clang. I
> closed most of them and tagged them as mistaken because pasting just
> warnings along with comments like "makes me nervous" do not make good bug
> reports.  It took me more than 2 hours to close them and briefly check
> some of them.  I might have missed some real things due to that torrent
> of reports, though. 
> 
> An example of faulty warnings is
> 
>   https://bugs.g10code.com/gnupg/issue1912
> 
> where one of the warnings is
> 
> iobuf.c:1325:3: warning: String copy function overflows destination buffer
>   strcpy (fcx->fname, fname);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Looking at the code in question:
> 
> --8<---------------cut here---------------start------------->8---
>   fcx = xmalloc (sizeof *fcx + strlen (fname));
>   fcx->fp = fp;
>   fcx->print_only_name = print_only;
>   strcpy (fcx->fname, fname);
> --8<---------------cut here---------------end--------------->8---
> 
> the last line is 1325.  It is pretty easy to see that FCX->FNAME has
> been allocated 3 lines earlier using a standard C pattern to use a
> dynamically sized struct (for FCX):
> 
> --8<---------------cut here---------------start------------->8---
>   typedef struct
>   {
>     gnupg_fd_t fp;       /* Open file pointer or handle.  */
>     int keep_open;
>     int no_cache;
>     int eof_seen;
>     int print_only_name; /* Flags indicating that fname [...]  */
>     char fname[1];       /* Name of the file.  */
>   } file_filter_ctx_t;
> --8<---------------cut here---------------end--------------->8---
> 
> The xmalloc (which is a common name for a malloc wrapper) allocates the
> size of the struct including the one byte for fname (from the
> definition) and adds the strlen of the SOURCE string to the allocation
> size.  Thus the total allocation for FCX->FNAME is
> 
>  (1 + strlen(SOURCE))
> 
> and a final strcpy works as expected without any overflow.
> 
> Now, how useful is an analyzer which does not grok one of the oldest C
> coding pattern?  Frankly, I have not seen that before and thus I assume
> that this and other warnings are due to a regression in that version of
> Clang or due to wrong use.
> 
> 
> Salam-Shalom,
> 
>    Werner
> 
> 

-- 
PGP fingerprint: 5E61 C878 0F86 295C E17D  8677 9F0F E587 374B BE81
https://pgp.mit.edu/pks/lookup?op=vindex&search=0x9F0FE587374BBE81