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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html