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

[Duy Nguyen <pclouds@gmail.com>]

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