Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jun 18, 2015, 10:13 UTC
- Message-ID
- <20150618101353.GA6525@lanh>
- In-Reply-To
- <20150617191235.GB25304@peff.net>
On Wed, Jun 17, 2015 at 03:12:35PM -0400, Jeff King wrote:
Show 14 quoted lines
> 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