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

[Stefan Beller <sbeller@google.com>]

On Wed, Jun 17, 2015 at 3:16 AM, Nguyễn Thái Ngọc Duy <pclouds@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 <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.


>
>  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