On Thu, Jun 18, 2015 at 09:46:09AM -0700, Junio C Hamano wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> 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
--
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