On Wed, Jun 17, 2015 at 3:16 AM, Nguyễn Thái Ngọc Duy <pclo...@gmail.com> wrote: > It usually goes like this > > strbuf sb = STRBUF_INIT; > if (!strncmp(sb.buf, "foo", 3)) > printf("%s", sb.buf + 3); > > Coverity thinks that printf() can be executed, and because initial > sb.buf only has one character (from strbuf_slopbuf), sb.buf + 3 is out > of bound. What it does not recognize is strbuf_slopbuf[0] is always (*) > zero. We always do some string comparison before jumping ahead to > "sb.buf + 3" and those operations will stop out of bound accesses. > > Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's > happy, we'll have cleaner defect list and better chances of spotting > true defects. > > (*) It's not entirely wrong though. Somebody may do sb.buf[0] = 'f' > right after variable declaration and ruin all unused strbuf. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com> > --- > There are lots of false warnings like this from Coverity. I just > wanted to kill them off so we can spot more serious problems easier. > I can't really verify that this patch shuts off those warnings > because scan.coverity.com policy does not allow forks.
Thanks a lot for looking into this! I'll just took this patch and have run a custom build now. (Depending on the outcome of the discussion, I may just carry this patch around on top of the origin/pu for each scan.) This patch is however a work around and not fixing the root problem. (The root problem being, coverity is not good enough to understand the implications of strncmp, skip_prefix, starts_with or such). The trade off is we're not able to detect problems with strbuf if any arise. > > I had another patch that avoids corrupting strbuf_slopbuf, by putting > it to .rodata section. The patch is more invasive though, because > this statement buf.buf[buf.len] = '\0' now has to make sure buf.buf > is not strbuf_slopbuf. It feels safer, but probably not enough to > justify the change. > > strbuf.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/strbuf.c b/strbuf.c > index 0d4f4e5..0d7c3cf 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -16,7 +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. > */ > +#ifndef __COVERITY__ > char strbuf_slopbuf[1]; > +#else > +/* Stop so many incorrect out-of-boundary warnings from Coverity */ > +char strbuf_slopbuf[64]; > +#endif > > void strbuf_init(struct strbuf *sb, size_t hint) > { > -- > 2.3.0.rc1.137.g477eb31 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html