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

[Duy Nguyen <pclouds@gmail.com>]

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