[PATCH] strbuf: stop out-of-boundary warnings from Coverity

[PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Nguyễn Thái Ngọc Duy]

It usually goes like this

    strbuf sb = STRBUF_INIT;
    if (!strncmp(sb.buf, "foo", 3))
       printf("%s", sb.buf + 3);

Coverity thinks that printf() can be executed, and because initial
sb.buf only has one character (from strbuf_slopbuf), sb.buf + 3 is out
of bound. What it does not recognize is strbuf_slopbuf[0] is always (*)
zero. We always do some string comparison before jumping ahead to
"sb.buf + 3" and those operations will stop out of bound accesses.

Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
happy, we'll have cleaner defect list and better chances of spotting
true defects.

(*) It's not entirely wrong though. Somebody may do sb.buf[0] = 'f'
    right after variable declaration and ruin all unused strbuf.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 There are lots of false warnings like this from Coverity. I just
 wanted to kill them off so we can spot more serious problems easier.
 I can't really verify that this patch shuts off those warnings
 because scan.coverity.com policy does not allow forks.

 I had another patch that avoids corrupting strbuf_slopbuf, by putting
 it to .rodata section. The patch is more invasive though, because
 this statement buf.buf[buf.len] = '\0' now has to make sure buf.buf
 is not strbuf_slopbuf. It feels safer, but probably not enough to
 justify the change.

 strbuf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 0d4f4e5..0d7c3cf 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -16,7 +16,12 @@ int starts_with(const char *str, const char *prefix)
  * buf is non NULL and ->buf is NUL terminated even for a freshly
  * initialized strbuf.
  */
+#ifndef __COVERITY__
 char strbuf_slopbuf[1];
+#else
+/* Stop so many incorrect out-of-boundary warnings from Coverity */
+char strbuf_slopbuf[64];
+#endif
 
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Stefan Beller]

On Wed, Jun 17, 2015 at 3:16 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> It usually goes like this
>
>     strbuf sb = STRBUF_INIT;
>     if (!strncmp(sb.buf, "foo", 3))
>        printf("%s", sb.buf + 3);
>
> Coverity thinks that printf() can be executed, and because initial
> sb.buf only has one character (from strbuf_slopbuf), sb.buf + 3 is out
> of bound. What it does not recognize is strbuf_slopbuf[0] is always (*)
> zero. We always do some string comparison before jumping ahead to
> "sb.buf + 3" and those operations will stop out of bound accesses.
>
> Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
> happy, we'll have cleaner defect list and better chances of spotting
> true defects.
>
> (*) It's not entirely wrong though. Somebody may do sb.buf[0] = 'f'
>     right after variable declaration and ruin all unused strbuf.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  There are lots of false warnings like this from Coverity. I just
>  wanted to kill them off so we can spot more serious problems easier.
>  I can't really verify that this patch shuts off those warnings
>  because scan.coverity.com policy does not allow forks.

Thanks a lot for looking into this!
I'll just took this patch and have run a custom build now.
(Depending on the outcome of the discussion, I may just carry
this patch around on top of the origin/pu for each scan.)

This patch is however a work around and not fixing the root problem.
(The root problem being, coverity is not good enough to understand the
implications of strncmp, skip_prefix, starts_with or such).

The trade off is we're not able to detect problems with strbuf if any arise.


>
>  I had another patch that avoids corrupting strbuf_slopbuf, by putting
>  it to .rodata section. The patch is more invasive though, because
>  this statement buf.buf[buf.len] = '\0' now has to make sure buf.buf
>  is not strbuf_slopbuf. It feels safer, but probably not enough to
>  justify the change.
>
>  strbuf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index 0d4f4e5..0d7c3cf 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -16,7 +16,12 @@ int starts_with(const char *str, const char *prefix)
>   * buf is non NULL and ->buf is NUL terminated even for a freshly
>   * initialized strbuf.
>   */
> +#ifndef __COVERITY__
>  char strbuf_slopbuf[1];
> +#else
> +/* Stop so many incorrect out-of-boundary warnings from Coverity */
> +char strbuf_slopbuf[64];
> +#endif
>
>  void strbuf_init(struct strbuf *sb, size_t hint)
>  {
> --
> 2.3.0.rc1.137.g477eb31
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Stefan Beller]

> Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
> happy, we'll have cleaner defect list

It's down 31 defects, roughly 10% of all things coverity detected as
problematic.
YAY!

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Jeff King]

On Wed, Jun 17, 2015 at 10:58:10AM -0700, Stefan Beller wrote:

> > Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
> > happy, we'll have cleaner defect list
> 
> It's down 31 defects, roughly 10% of all things coverity detected as
> problematic.
> YAY!

That's a good thing.  I do find the solution a little gross, though. I
wonder if there is a way we can tell coverity more about how strbuf
works. I've noticed similar problems with string_list, where it
complains that we are touching list->items, which was assigned to NULL
(of course it was, but then after that we did string_list_append!).

I know literally nothing about coverity's annotations and what's
possible with them. So I may be barking up a wrong tree completely.

-Peff

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Stefan Beller]

On Wed, Jun 17, 2015 at 12:12 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jun 17, 2015 at 10:58:10AM -0700, Stefan Beller wrote:
>
>> > Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
>> > happy, we'll have cleaner defect list
>>
>> It's down 31 defects, roughly 10% of all things coverity detected as
>> problematic.
>> YAY!
>
> That's a good thing.  I do find the solution a little gross, though. I
> wonder if there is a way we can tell coverity more about how strbuf
> works.

I always thought the problem was a combination of both having a custom
strcmp (like skip_prefix, starts_with) and a custom data structure (strbuf,
string_list). So I am not sure if it is sufficient to tell coverity

> I've noticed similar problems with string_list, where it
> complains that we are touching list->items, which was assigned to NULL
> (of course it was, but then after that we did string_list_append!).
>
> I know literally nothing about coverity's annotations and what's
> possible with them. So I may be barking up a wrong tree completely.

I have searched for the exact annotations for a while, but all I found
were examples in other open source projects, no official documentation
with all its features. I may be missing something though (there must be
some official documentation, I'd assume).

>
> -Peff

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Duy Nguyen]

On Wed, Jun 17, 2015 at 03:12:35PM -0400, Jeff King wrote:
> On Wed, Jun 17, 2015 at 10:58:10AM -0700, Stefan Beller wrote:
> 
> > > Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
> > > happy, we'll have cleaner defect list
> > 
> > It's down 31 defects, roughly 10% of all things coverity detected as
> > problematic.
> > YAY!
> 
> That's a good thing.  I do find the solution a little gross, though. I
> wonder if there is a way we can tell coverity more about how strbuf
> works. I've noticed similar problems with string_list, where it
> complains that we are touching list->items, which was assigned to NULL
> (of course it was, but then after that we did string_list_append!).

There's "function modeling" where you write simplified (and likely
incorrect) version of a function to correct how coverity's
understanding of that function. I searched, there's no "data
modeling". I think I have the user manual, but haven't looked through
it yet.

On Thu, Jun 18, 2015 at 2:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I actually think this is too ugly to live.

Well, I have another option below. Let's see how people feel about it.

> If coverity is buggy and unusable, why aren't we raising that issue
> to them?

It's technically correct though. The key point here is strbuf_slopbuf[0]
is NUL, but that cannot be statically determined. And we may also need
to teach it about strcmp' and friends' semantics. That's probably too
much for a static analyzer.

The last resort is simply filter out a whole class of warnings.
Probably good enough if both patches look equally ugly.

-- 8< --
Subject: [PATCH] strbuf: kill strbuf_slopbuf, in favor of ""

A lot of "out-of-bound access" warnings on scan.coverity.com is because
it does not realize this strbuf_slopbuf[] is in fact initialized with a
single and promised to never change. But that promise could be broken if
some caller attempts to write to strbuf->buf[0] write after STRBUF_INIT.

We really can't do much about it. But we can try to put strbuf_slopbuf
in .rodata section, where writes will be caught by the OS with memory
protection support. The only drawback is people can't do
"buf->buf == strbuf_slopbuf" any more. Luckily nobody does that in the
current code base.
---
 strbuf.c | 19 ++++++-------------
 strbuf.h | 11 ++++++++---
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 0d4f4e5..9f91229 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,17 +11,10 @@ int starts_with(const char *str, const char *prefix)
 			return 0;
 }
 
-/*
- * Used as the default ->buf value, so that people can always assume
- * buf is non NULL and ->buf is NUL terminated even for a freshly
- * initialized strbuf.
- */
-char strbuf_slopbuf[1];
-
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
 	sb->alloc = sb->len = 0;
-	sb->buf = strbuf_slopbuf;
+	sb->buf = (char*)"";
 	if (hint)
 		strbuf_grow(sb, hint);
 }
@@ -52,7 +45,7 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
 	sb->len   = len;
 	sb->alloc = alloc;
 	strbuf_grow(sb, 0);
-	sb->buf[sb->len] = '\0';
+	strbuf_terminate(sb);
 }
 
 void strbuf_grow(struct strbuf *sb, size_t extra)
@@ -77,7 +70,7 @@ void strbuf_rtrim(struct strbuf *sb)
 {
 	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
-	sb->buf[sb->len] = '\0';
+	strbuf_terminate(sb);
 }
 
 void strbuf_ltrim(struct strbuf *sb)
@@ -88,7 +81,7 @@ void strbuf_ltrim(struct strbuf *sb)
 		sb->len--;
 	}
 	memmove(sb->buf, b, sb->len);
-	sb->buf[sb->len] = '\0';
+	strbuf_terminate(sb);
 }
 
 int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
@@ -380,7 +373,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 		strbuf_grow(sb, 8192);
 	}
 
-	sb->buf[sb->len] = '\0';
+	strbuf_terminate(sb);
 	return sb->len - oldlen;
 }
 
@@ -496,7 +489,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	if (ch == EOF && sb->len == 0)
 		return EOF;
 
-	sb->buf[sb->len] = '\0';
+	strbuf_terminate(sb);
 	return 0;
 }
 #endif
diff --git a/strbuf.h b/strbuf.h
index 01c5c63..d8346ee 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,8 +67,13 @@ struct strbuf {
 	char *buf;
 };
 
-extern char strbuf_slopbuf[];
-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { 0, 0, (char*)"" }
+
+static inline void strbuf_terminate(struct strbuf *sb)
+{
+	if (sb->buf[sb->len])
+		sb->buf[sb->len] = '\0';
+}
 
 /**
  * Life Cycle Functions
@@ -149,7 +154,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
 		die("BUG: strbuf_setlen() beyond buffer");
 	sb->len = len;
-	sb->buf[len] = '\0';
+	strbuf_terminate(sb);
 }
 
 /**
-- 
2.3.0.rc1.137.g477eb31
-- 8< --
-- 
Duy

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> The last resort is simply filter out a whole class of warnings.
> Probably good enough if both patches look equally ugly.
>
> -- 8< --
> Subject: [PATCH] strbuf: kill strbuf_slopbuf, in favor of ""
>
> A lot of "out-of-bound access" warnings on scan.coverity.com is because
> it does not realize this strbuf_slopbuf[] is in fact initialized with a
> single and promised to never change. But that promise could be broken if
> some caller attempts to write to strbuf->buf[0] write after STRBUF_INIT.
>
> We really can't do much about it. But we can try to put strbuf_slopbuf
> in .rodata section, where writes will be caught by the OS with memory
> protection support. The only drawback is people can't do
> "buf->buf == strbuf_slopbuf" any more. Luckily nobody does that in the
> current code base.
> ---

Hmph, would declaring slopbuf as "const char [1]" (and sprinkling
the "(char *)" cast) have the same effect, I wonder?

> +static inline void strbuf_terminate(struct strbuf *sb)
> +{
> +	if (sb->buf[sb->len])
> +		sb->buf[sb->len] = '\0';
> +}

This is so that you can call things like strbuf_rtrim() immediately
after running strbuf_init() safely, but I think it needs a comment
to save people from wondering what is going on, e.g. "this is not an
optimization to avoid assigning NUL to a place that is already NUL;
a freshly initialized strbuf points at an unwritable piece of NUL
and we do not want to cause a SEGV".

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Duy Nguyen]

On Thu, Jun 18, 2015 at 09:46:09AM -0700, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > The last resort is simply filter out a whole class of warnings.
> > Probably good enough if both patches look equally ugly.
> >
> > -- 8< --
> > Subject: [PATCH] strbuf: kill strbuf_slopbuf, in favor of ""
> >
> > A lot of "out-of-bound access" warnings on scan.coverity.com is because
> > it does not realize this strbuf_slopbuf[] is in fact initialized with a
> > single and promised to never change. But that promise could be broken if
> > some caller attempts to write to strbuf->buf[0] write after STRBUF_INIT.
> >
> > We really can't do much about it. But we can try to put strbuf_slopbuf
> > in .rodata section, where writes will be caught by the OS with memory
> > protection support. The only drawback is people can't do
> > "buf->buf == strbuf_slopbuf" any more. Luckily nobody does that in the
> > current code base.
> > ---
> 
> Hmph, would declaring slopbuf as "const char [1]" (and sprinkling
> the "(char *)" cast) have the same effect, I wonder?

I remember I tried that and failed with a compile error. I just tried
again, this time it worked. Must have made some stupid mistake in my
first try.

Anyway it does not put strbuf_slopbuf in .rodata. "./git" does not
segfault with this patch

-- 8< --
diff --git a/git.c b/git.c
index 44374b1..960e375 100644
--- a/git.c
+++ b/git.c
@@ -621,7 +621,11 @@ int main(int argc, char **av)
 	const char **argv = (const char **) av;
 	const char *cmd;
 	int done_help = 0;
+	struct strbuf sb = STRBUF_INIT;
 
+	sb.buf[0] = 'Z';
+	printf("%c\n", strbuf_slopbuf[0]);
+	return 0;
 	startup_info = &git_startup_info;
 
 	cmd = git_extract_argv0_path(argv[0]);
diff --git a/strbuf.c b/strbuf.c
index 0d4f4e5..3a0d4c9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -16,12 +16,12 @@ int starts_with(const char *str, const char *prefix)
  * buf is non NULL and ->buf is NUL terminated even for a freshly
  * initialized strbuf.
  */
-char strbuf_slopbuf[1];
+const char strbuf_slopbuf[1];
 
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
 	sb->alloc = sb->len = 0;
-	sb->buf = strbuf_slopbuf;
+	sb->buf = (char *)strbuf_slopbuf;
 	if (hint)
 		strbuf_grow(sb, hint);
 }
diff --git a/strbuf.h b/strbuf.h
index 01c5c63..eab7307 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,8 +67,8 @@ struct strbuf {
 	char *buf;
 };
 
-extern char strbuf_slopbuf[];
-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+extern const char strbuf_slopbuf[];
+#define STRBUF_INIT  { 0, 0, (char *)strbuf_slopbuf }
 
 /**
  * Life Cycle Functions
-- 8< --

> > +static inline void strbuf_terminate(struct strbuf *sb)
> > +{
> > +	if (sb->buf[sb->len])
> > +		sb->buf[sb->len] = '\0';
> > +}
> 
> This is so that you can call things like strbuf_rtrim() immediately
> after running strbuf_init() safely, but I think it needs a comment
> to save people from wondering what is going on, e.g. "this is not an
> optimization to avoid assigning NUL to a place that is already NUL;
> a freshly initialized strbuf points at an unwritable piece of NUL
> and we do not want to cause a SEGV".

Sure, if we go with this direction.
--
Duy

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Remi Galan Alfonso]

Duy Nguyen <pclouds@gmail.com> writes:

> + sb.buf[0] = 'Z';
> + printf("%c\n", strbuf_slopbuf[0]);
> + return 0;
> startup_info = &git_startup_info;

I might be wrong, but I definitely think that this
printf and return 0 are some debug lines that you 
forgot to remove.

Rémi

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Duy Nguyen]

On Fri, Jun 19, 2015 at 5:50 PM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> + sb.buf[0] = 'Z';
>> + printf("%c\n", strbuf_slopbuf[0]);
>> + return 0;
>> startup_info = &git_startup_info;
>
> I might be wrong, but I definitely think that this
> printf and return 0 are some debug lines that you
> forgot to remove.

Yes it's debug code. I hoped that if I made a mistake forcing the
segfault, people would notice.
-- 
Duy

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Anyway it does not put strbuf_slopbuf in .rodata.

That's sad.  I wa hoping that it would behave the same as this,
which does give me SEGV:

#include <stdio.h>

static const char x = '\0';
static char *y = (char *)&x;

int main (void) {
	*y = 1;
}

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
>> happy, we'll have cleaner defect list
>
> It's down 31 defects, roughly 10% of all things coverity detected as
> problematic.
> YAY!

I actually think this is too ugly to live.  If coverity is buggy and
unusable, why aren't we raising that issue to them?

Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity

[Stefan Beller]

On Wed, Jun 17, 2015 at 12:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
>>> happy, we'll have cleaner defect list
>>
>> It's down 31 defects, roughly 10% of all things coverity detected as
>> problematic.
>> YAY!
>
> I actually think this is too ugly to live.  If coverity is buggy and
> unusable, why aren't we raising that issue to them?

We can try to do that.
The last time I wanted them to take a look at Git, they were
unresponsive. I presume that's what you get when not being
a paying customer. :(