Re: Fw: New Defects reported by Coverity Scan for git [argv_array: offer to split a string by whitespace]

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 23, 2018, 11:07 UTC
Message-ID: <nycvar.QRO.7.76.6.1804231307090.15174@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <04F34371EF174E1F8E6E762D1BCA6C4F@PhilipOakley>

Hi Philip,

On Sun, 22 Apr 2018, Philip Oakley wrote:

Show 34 quoted lines

> is this part of your series "argv_array: offer to split a string by
> whitespace"?
> 
> https://public-inbox.org/git/CAPig+cTDbTtUeFYmkNtM773EBgE14Tpic4g4XEFuSVwSypdMjw@mail.gmail.com/
> 
> ----- Original Message ----- From: <scan-admin@coverity.com>
> Sent: Saturday, April 21, 2018 10:53 AM
> Subject: New Defects reported by Coverity Scan for git
> 
> > New defect(s) Reported-by: Coverity Scan
> > Showing 1 of 1 defect(s)
> >
> >
> > ** CID 1434982:  Memory - corruptions  (OVERRUN)
> >
> >
> > ________________________________________________________________________________________________________
> > *** CID 1434982:  Memory - corruptions  (OVERRUN)
> > /builtin/replace.c: 475 in convert_graft_file()
> > 469
> > 470     while (strbuf_getline(&buf, fp) != EOF) {
> > 471     if (*buf.buf == '#')
> > 472     continue;
> > 473
> > 474     argv_array_split(&args, buf.buf);
> > > > >     CID 1434982:  Memory - corruptions  (OVERRUN)
> > > > >     Overrunning buffer pointed to by "args.argv" of 8 bytes by passing
> > > > > it to a function which accesses it at byte offset 8.
> > 475     if (args.argc && create_graft(args.argc, args.argv, force))
> > 476     strbuf_addf(&err, "\n\t%s", buf.buf);
> > 477     argv_array_clear(&args);
> > 478     }
> > 479
> > 480     strbuf_release(&buf);

Yes, it is. Coverity has problems to figure out what is really happening here, and it has the exact same problems with strbufs.

We initialize both of these structs using static initializers, with specific, empty arrays. When we need to reallocate, we figure out that the empty array was still there and replace it with a NULL so we can realloc. So there is no buffer overrun, but Coverity cannot figure that out, and as much as I tried, I could not come up with a "template" to shut up Coverity.

Ciao, Dscho