Re: Covierty Integration / Improvement

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 7, 2022, 11:49 UTC
Message-ID: <nycvar.QRO.7.76.6.2204071344210.347@tvgsbejvaqbjf.bet>
In-Reply-To: <Yk3UAz3sn9KhMnyf@mit.edu>

Hi Theodore,

On Wed, 6 Apr 2022, Theodore Ts'o wrote:

Show 12 quoted lines

> On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:
> > I have fixed Git for Windows' Coverity build and started to sift through
> > the 154 new defects reported as of v2.36.0-rc0.
> >
> > Sadly, there is now a new class of overwhelming false positives: Coverity
> > claims that "strbuf_addstr does not [NUL-]terminate", which is of course
> > false.
>
> It should be possible to suppress this by uploading a Coverity model
> file.  See[1] for more details:
>
> [1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model

Right, I know about this, my apologies for being too succinct. For the record, here is how we submit the Coverity builds in Git for Windows:

https://github.com/git-for-windows/build-extra/blob/bea0c37bdc3737843b7808269b077b30e51c67fb/please.sh#L1793-L1925

Unfortunately, you cannot see the model file because that is not provided per build, but globally for the entire project. Here is the model file we currently use:

-- snip -- /* modelfile for git */

char strbuf_slopbuf[64];

void *malloc(size_t); void *calloc(size_t, size_t); void *realloc(void *, size_t); void free(void *);

void *xrealloc(void *ptr, size_t size)
{
	void *ret = realloc(ptr, size);
	if (!ret) __coverity_panic__();
	return ret;
}

void *xmalloc(size_t size)
{
	void *mem = malloc(size);
	if (!mem) __coverity_panic__();
	return mem;
}

void xcalloc(size_t num, size_t size)
{
	void *ret = calloc(num, size);
	if (!ret)  __coverity_panic__();
	return ret;
}

void usage(const char *err) {
  __coverity_panic__();
}

void usagef(const char *err, ...) {
  __coverity_panic__();
}

void die(const char *err, ...)  {
  __coverity_panic__();
}

void die_errno(const char *err, ...) {
  __coverity_panic__();
}
-- snap --

I _guess_ we could "help" Coverity by providing some alternative implementation of `strbuf_add()` that does not use `memcpy()` but instead a loop that explicitly NUL-terminates the string.

But it feels wrong to do that because that would weaken the analysis because Coverity would not actually analyze the code that is executed anymore.

Show 30 quoted lines

> I've suppressed a similar issue by using the attribute __nonstring,
> but I don't think that will work for git, because strbuf->buf really
> *is* a NUL-terminated string, where as in ext4 we have some fields
> which are designed to be NUL padded, but it is *not* guaranteed to be
> NUL-terminated:
>
> #ifndef __nonstring
> #ifdef __has_attribute
> #if __has_attribute(__nonstring__)
> #define __nonstring                    __attribute__((__nonstring__))
> #else
> #define __nonstring
> #endif /* __has_attribute(__nonstring__) */
> #else
> # define __nonstring
> #endif /* __has_attribute */
> #endif /* __nonstring */
>
> struct ext2_super_block {
>        ...
> /*068*/	__u8	s_uuid[16] __nonstring;		/* 128-bit uuid for volume */
> /*078*/	__u8	s_volume_name[EXT2_LABEL_LEN] __nonstring;	/* volume name */
>        ...
> };
>
> (This is needed to suppress warnings by Clang as well.)
>
> Using __nonstring will result in attempts to use s_volume_name in "C"
> string context to give a warning, which is why this isn't right for
> strbuf->buf.

Correct. I guess we could fool around with the model file until those false positives are gone, but I have to admit that I cannot justify the time to work on this.

Ciao, Johannes