Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity
- From
Stefan Beller <sbeller@google.com>
- Date
- Jun 17, 2015, 17:28 UTC
- Message-ID
- <CAGZ79kYRfjeXGkYAv-Kn2Bk-pp2ZSzpKGHDhqMpw03scdRZAmQ@mail.gmail.com>
- In-Reply-To
- <1434536209-31350-1-git-send-email-pclouds@gmail.com>
On Wed, Jun 17, 2015 at 3:16 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 25 quoted lines
> 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 <pclouds@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.
Show 34 quoted lines
>
> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html