Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Stefan, On Wed, 10 May 2017, Stefan Beller wrote: > On Wed, May 10, 2017 at 12:48 PM, Johannes Schindelin > wrote: > > > > On Fri, 5 May 2017, Johannes Schindelin wrote: > > > >> On Fri, 28 Apr 2017, Johannes Schindelin wrote: > >> > >> > On Fri, 28 Apr 2017, Stefan Beller wrote: > >> > > >> > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin > >> > > wrote: > >> > > > >> > > > I still have to find the time to figure out one more detail: how > >> > > > to download and extract the Coverity tool (the .zip archive has a > >> > > > variable name for the top-level directory), and doing that only > >> > > > every once in a while, say, only when there is no previously > >> > > > unpacked tool, or it is already 4 weeks old. > >> > > > >> > > That is an interesting problem, which I ignored as the older > >> > > versions of their tools still works once they release new versions. > >> > > So I just manually check every once in a while if they have new > >> > > versions out there. > >> > > > >> > > So if you find a nice solution to that problem, let me know, please. > >> > > >> > I think I have a working idea (jotting it down in the editor, > >> > untested): > >> > > >> > [... totally untested snippet ...] > >> > >> And now I edited it and tested it. The code is now part of the script I > >> use for pretty much all administrative (i.e. recurring and boring) tasks > >> in the Git for Windows project: > >> > >> https://github.com/git-for-windows/build-extra/commit/05b5342128 > > > > Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY > > constant to something quite large (I used 64k), but apparently that does > > not work as expected, Coverity still insists on complaining about strbufs. > > > > On a second thought, it is actually quite obvious why it does not fix > > those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh. > > D'oh. I must have been living in an alternate universe for quite some time > as I seemed to have confused different things here. > > Checkout this commit, > https://github.com/stefanbeller/git/commit/977f81d6dec4461a1a12da2df6c5c919b41129b4 > that is cherry-picked for the coverity build. That fixes it. I saw that patch, and since Junio did not pick it up, I figured that it won't make it into git.git's source code. That's why I went with the `sed` approach, as the #ifndef __COVERITY__ guard is not even necessary if I have to patch the code before running the Coverity tool anyway. > That is about the only patch I apply before sending it off to coverity. > I am still contemplating a nice solution for FLEX_ARRAYs in other cases. For FLEX_ARRAYs, I use this: cov-build --dir cov-int \ make -j15 DEVELOPER=1 CPPFLAGS=-DFLEX_ARRAY=65536 I have not had time to go over more than half a dozen CIDs, but I *think* it helps. Ciao, Dscho
Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
On Wed, May 10, 2017 at 12:48 PM, Johannes Schindelin wrote: > Hi Stefan, > > On Fri, 5 May 2017, Johannes Schindelin wrote: > >> On Fri, 28 Apr 2017, Johannes Schindelin wrote: >> >> > On Fri, 28 Apr 2017, Stefan Beller wrote: >> > >> > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin >> > > wrote: >> > > >> > > > I still have to find the time to figure out one more detail: how >> > > > to download and extract the Coverity tool (the .zip archive has a >> > > > variable name for the top-level directory), and doing that only >> > > > every once in a while, say, only when there is no previously >> > > > unpacked tool, or it is already 4 weeks old. >> > > >> > > That is an interesting problem, which I ignored as the older >> > > versions of their tools still works once they release new versions. >> > > So I just manually check every once in a while if they have new >> > > versions out there. >> > > >> > > So if you find a nice solution to that problem, let me know, please. >> > >> > I think I have a working idea (jotting it down in the editor, >> > untested): >> > >> > [... totally untested snippet ...] >> >> And now I edited it and tested it. The code is now part of the script I >> use for pretty much all administrative (i.e. recurring and boring) tasks >> in the Git for Windows project: >> >> https://github.com/git-for-windows/build-extra/commit/05b5342128 > > Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY > constant to something quite large (I used 64k), but apparently that does > not work as expected, Coverity still insists on complaining about strbufs. > > On a second thought, it is actually quite obvious why it does not fix > those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh. D'oh. I must have been living in an alternate universe for quite some time as I seemed to have confused different things here. Checkout this commit, https://github.com/stefanbeller/git/commit/977f81d6dec4461a1a12da2df6c5c919b41129b4 that is cherry-picked for the coverity build. That fixes it. That is about the only patch I apply before sending it off to coverity. I am still contemplating a nice solution for FLEX_ARRAYs in other cases. Thanks, Stefan
Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Stefan, On Fri, 5 May 2017, Johannes Schindelin wrote: > On Fri, 28 Apr 2017, Johannes Schindelin wrote: > > > On Fri, 28 Apr 2017, Stefan Beller wrote: > > > > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin > > > wrote: > > > > > > > I still have to find the time to figure out one more detail: how > > > > to download and extract the Coverity tool (the .zip archive has a > > > > variable name for the top-level directory), and doing that only > > > > every once in a while, say, only when there is no previously > > > > unpacked tool, or it is already 4 weeks old. > > > > > > That is an interesting problem, which I ignored as the older > > > versions of their tools still works once they release new versions. > > > So I just manually check every once in a while if they have new > > > versions out there. > > > > > > So if you find a nice solution to that problem, let me know, please. > > > > I think I have a working idea (jotting it down in the editor, > > untested): > > > > [... totally untested snippet ...] > > And now I edited it and tested it. The code is now part of the script I > use for pretty much all administrative (i.e. recurring and boring) tasks > in the Git for Windows project: > > https://github.com/git-for-windows/build-extra/commit/05b5342128 Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY constant to something quite large (I used 64k), but apparently that does not work as expected, Coverity still insists on complaining about strbufs. On a second thought, it is actually quite obvious why it does not fix those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh. My next attempt to work around these bogus claims was to modify the "model file" by adding a line saying "char strbuf_slopbuf[64];", but that was sadly not picked up by Coverity either. My current thinking is that I will simply patch strbuf.c via `sed 's/^\(char struct_slopbuf\[\)1\[/&64[/'`. Ciao, Dscho
Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Stefan, On Fri, 28 Apr 2017, Johannes Schindelin wrote: > On Fri, 28 Apr 2017, Stefan Beller wrote: > > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin > > wrote: > > > > > I still have to find the time to figure out one more detail: how to > > > download and extract the Coverity tool (the .zip archive has a > > > variable name for the top-level directory), and doing that only > > > every once in a while, say, only when there is no previously > > > unpacked tool, or it is already 4 weeks old. > > > > That is an interesting problem, which I ignored as the older versions > > of their tools still works once they release new versions. So I just > > manually check every once in a while if they have new versions out > > there. > > > > So if you find a nice solution to that problem, let me know, please. > > I think I have a working idea (jotting it down in the editor, untested): > > [... totally untested snippet ...] And now I edited it and tested it. The code is now part of the script I use for pretty much all administrative (i.e. recurring and boring) tasks in the Git for Windows project: https://github.com/git-for-windows/build-extra/commit/05b5342128 May it be useful to you, too, at least as a starting point, Dscho
Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Lars, On Mon, 1 May 2017, Lars Schneider wrote: > Looks like Coverity has TravisCI integration and I assume you wouldn't > need to worry about downloading the tool in that setup: > https://scan.coverity.com/travis_ci Except for that tiny little fact that Travis CI does not support Windows backends (nor would they even respond to my offer to work with them). Ciao, Dscho
Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
> On 28 Apr 2017, at 22:29, Johannes Schindelin
> wrote:
>
> Hi Stefan,
>
> On Fri, 28 Apr 2017, Stefan Beller wrote:
>
>> On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
>> wrote:
>>
>>> I still have to find the time to figure out one more detail: how to
>>> download and extract the Coverity tool (the .zip archive has a
>>> variable name for the top-level directory), and doing that only every
>>> once in a while, say, only when there is no previously unpacked tool,
>>> or it is already 4 weeks old.
>>
>> That is an interesting problem, which I ignored as the older versions of
>> their tools still works once they release new versions. So I just
>> manually check every once in a while if they have new versions out
>> there.
>>
>> So if you find a nice solution to that problem, let me know, please.
>
> I think I have a working idea (jotting it down in the editor, untested):
>
> init_or_update_coverity_tool () {
> # check once per week whether there is a new version
> coverity_tool=.git/coverity-tool/
> test ! -d $coverity_tool ||
> test $(($(date +%s)-$(stat -c %Y $coverity_tool))) -gt
> $((7*24*60*60)) ||
> return
>
> curl --form "token=$(COVERITY.TOKEN)" \
> --form "project=git-for-windows" \
> --time-cond .git/coverity_tool.zip \
> -o .git/coverity_tool.zip.new \
> https://scan.coverity.com/download/win64 &&
> test -f .git/coverity_tool.zip.new || {
> # Try again in a week
> touch $coverity_tool
> return
> }
>
> mv -f .git/coverity_tool.zip.new .git/coverity_tool.zip ||
> die "Could not overwrite coverity_tool.zip"
>
> mkdir $coverity_tool.new &&
> (cd $coverity_tool.new &&
>unzip ../coverity_tool.zip) ||
> die "Could not unpack coverity_tool.zip"
>
> rm -rf $coverity_tool &&
> mv $coverity_tool.new $coverity_tool ||
> die "Could not switch to new Coverity tool version"
> }
>
> init_or_update_coverity_tool
> PATH=$(echo $coverity_tool/*/bin):$PATH
>
> I guess I will start from that snippet once I have time to work on that
> Coverity automation.
>
> BTW I stumbled over an interesting tidbit today: if you define FLEX_ARRAY
> outside of git-compat-util.h, it will not be overridden by Git. That is,
> if you want to use 64kB flex arrays by default, you can call
>
> make CPPFLAGS=-DFLEX_ARRAY=65536
>
> No need to patch the source code.
Looks like Coverity has TravisCI integration and I assume you wouldn't
need to worry about downloading the tool in that setup:
https://scan.coverity.com/travis_ci
I think we should be able to enable it without trouble for the 'master' branch?
Cheers,
Lars
Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Stefan,
On Fri, 28 Apr 2017, Stefan Beller wrote:
> On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
> wrote:
>
> > I still have to find the time to figure out one more detail: how to
> > download and extract the Coverity tool (the .zip archive has a
> > variable name for the top-level directory), and doing that only every
> > once in a while, say, only when there is no previously unpacked tool,
> > or it is already 4 weeks old.
>
> That is an interesting problem, which I ignored as the older versions of
> their tools still works once they release new versions. So I just
> manually check every once in a while if they have new versions out
> there.
>
> So if you find a nice solution to that problem, let me know, please.
I think I have a working idea (jotting it down in the editor, untested):
init_or_update_coverity_tool () {
# check once per week whether there is a new version
coverity_tool=.git/coverity-tool/
test ! -d $coverity_tool ||
test $(($(date +%s)-$(stat -c %Y $coverity_tool))) -gt
$((7*24*60*60)) ||
return
curl --form "token=$(COVERITY.TOKEN)" \
--form "project=git-for-windows" \
--time-cond .git/coverity_tool.zip \
-o .git/coverity_tool.zip.new \
https://scan.coverity.com/download/win64 &&
test -f .git/coverity_tool.zip.new || {
# Try again in a week
touch $coverity_tool
return
}
mv -f .git/coverity_tool.zip.new .git/coverity_tool.zip ||
die "Could not overwrite coverity_tool.zip"
mkdir $coverity_tool.new &&
(cd $coverity_tool.new &&
unzip ../coverity_tool.zip) ||
die "Could not unpack coverity_tool.zip"
rm -rf $coverity_tool &&
mv $coverity_tool.new $coverity_tool ||
die "Could not switch to new Coverity tool version"
}
init_or_update_coverity_tool
PATH=$(echo $coverity_tool/*/bin):$PATH
I guess I will start from that snippet once I have time to work on that
Coverity automation.
BTW I stumbled over an interesting tidbit today: if you define FLEX_ARRAY
outside of git-compat-util.h, it will not be overridden by Git. That is,
if you want to use 64kB flex arrays by default, you can call
make CPPFLAGS=-DFLEX_ARRAY=65536
No need to patch the source code.
Ciao,
Dscho
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Johannes, thanks for the reply. On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin wrote: > I still have to find the time to figure out one more detail: how to > download and extract the Coverity tool (the .zip archive has a variable > name for the top-level directory), and doing that only every once > in a while, say, only when there is no previously unpacked tool, or it is > already 4 weeks old. That is an interesting problem, which I ignored as the older versions of their tools still works once they release new versions. So I just manually check every once in a while if they have new versions out there. So if you find a nice solution to that problem, let me know, please. Thanks, Stefan
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Hannes, On Thu, 27 Apr 2017, Johannes Sixt wrote: > Am 26.04.2017 um 22:19 schrieb Johannes Schindelin: > > I recently registered the git-for-windows fork with Coverity to ensure > > that even the Windows-specific patches get some static analysis love. > > > > While at it, I squashed a couple of obvious issues in the part that is > > not Windows-specific. > > Thanks for the fish. I'd looked at the series and had a few comments. > > Hunting memory leaks is the way to insanity. I hear you. Loud and clear. > Never again am I going to help with this. Awww? And here I thought I had your attention... *sniffle* ;-) > I prefer to rewrite this codebase to C++ and have leak-free code by > design. I had the pleasure of working with some software developers in 2004 who were experts at introducing memory leaks into C++ code. The same bunch of people later produced Java code that sorted a string list in cubic time (carefully avoiding java.util.Collections.sort(), of course). For every fool-proof system invented, somebody invents a better fool. Ciao, Dscho
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Hi Stefan, On Wed, 26 Apr 2017, Stefan Beller wrote: > On Wed, Apr 26, 2017 at 1:19 PM, Johannes Schindelin > 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? For starters, I did everything manually. But I already have the grand plan to use VSTS build (I do not want to advertise this system too blatantly, but I have to say that I really love working with that system, it is so much more powerful than Travis or AppVeyor, Jenkins comes closer but not by far). As you may have glimpsed in one of my mails, Git for Windows needs a very special and extensive build setup. I call it the "Git for Windows SDK", and what it is, in a nutshell, is a custom MSYS2 installation that has all the packages installed that are needed to build Git for Windows, its most important dependencies, and to bundle everything in an installer. Think of MSYS2 as something like Homebrew. But cozier, as it really lives inside its own directory tree, its /etc/ is not really visible to any other software, for example. This makes Continuous Testing a bit of a challenge, as you cannot hammer the package repository of MSYS2 with tons of fresh full installations all the time (that package repository is run by a volunteer, and it is sometimes unreachable, too). So after talking to a GitHub guy at the Developer Summit during GitMerge (Patrick asked a couple of questions and then said my plan was okay), I put two new repositories up that host the initialized Git for Windows SDKs (one for 32-bit, one for 64-bit). And of course there is a job to keep them up-to-date, run once every 24h. Almost all of the VSTS builds I maintain clone or update one or both of these repositories. And so will also the Coverity job. I still have to find the time to figure out one more detail: how to download and extract the Coverity tool (the .zip archive has a variable name for the top-level directory), and doing that only every once in a while, say, only when there is no previously unpacked tool, or it is already 4 weeks old. And then I should probably try to figure out a way how to delay subsequent runs of that job so that we run it at most once every twenty-four hours. Ciao, Dscho
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
Am 26.04.2017 um 22:19 schrieb Johannes Schindelin: I recently registered the git-for-windows fork with Coverity to ensure that even the Windows-specific patches get some static analysis love. While at it, I squashed a couple of obvious issues in the part that is not Windows-specific. Thanks for the fish. I'd looked at the series and had a few comments. Hunting memory leaks is the way to insanity. Never again am I going to help with this. I prefer to rewrite this codebase to C++ and have leak-free code by design. Thanks, -- Hannes
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
On Wed, Apr 26, 2017 at 1:19 PM, Johannes Schindelin 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
