Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Stefan Beller <sbeller@google.com>]

On Wed, Apr 26, 2017 at 1:19 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> I recently registered the git-for-windows fork with Coverity to ensure
> that even the Windows-specific patches get some static analysis love.

YAY! How do you trigger the coverity scan? (via travis or uploading a tarball
manually every once in a while?) Can you talk about the setup? I'm interested
in that as I run coverity for Junios pu branch, plus a couple patches on top[1].

[1] https://github.com/stefanbeller/git/commits/coverity

>
> While at it, I squashed a couple of obvious issues in the part that is
> not Windows-specific.

Thanks.

> Note: while this patch series squashes some of those issues, the
> remaining issues are not necessarily all false positives (e.g. Coverity
> getting fooled by our FLEX_ARRAY trick into believing that the last
> member of the struct is indeed a 0 or 1 size array) or intentional (e.g.
> builtins not releasing memory because exit() is called right after
> returning from the function that leaks memory).

For the latter I think we could get away with maintaining patches on
a dedicated coverity branch (which I do currently, just not in an extensive
manner). For the first, I think we can just compile with FLEX_ARRAY
defined as an insanely large number. I'll experiment with that.

> Notable examples of the remaining issues are:
>
> - a couple of callers of shorten_unambiguous_ref() assume that they do
>   not have to release the memory returned from that function, often
>   assigning the pointer to a `const` variable that typically does not
>   hold a pointer that needs to be free()d. My hunch is that we will want
>   to convert that function to have a fixed number of static buffers and
>   use those in a round robin fashion à la sha1_to_hex().
>
> - pack-redundant.c seems to have hard-to-reason-about code paths that
>   may eventually leak memory. Essentially, the custody of the allocated
>   memory is not clear at all.
>
> - fast-import.c calls strbuf_detach() on the command_buf without any
>   obvious rationale. Turns out that *some* code paths assign
>   command_buf.buf to a `recent_command` which releases the buffer later.
>   However, from a cursory look it appears to me as if there are some
>   other code paths that skip that assignment, effectively causing a memory
>   leak once strbuf_detach() is called.
>
> Sadly, I lack the time to pursue those remaining issues further.
>

Thanks,
Stefan