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

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

[Johannes Schindelin]

Hi Philip,

On Sun, 22 Apr 2018, Philip Oakley wrote:

> 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