travis: war on leaks #4558

tiennou · 2018-02-28T21:00:38Z

This should make Travis fail the build when leaks are detected.

~~There's a prototype implementation on osx, because I just noticed leaks gained an -atExit argument, which I can't test locally for update-laziness reasons.~~ Removed for the time being because it depends on undocumentation.

Supersedes #4416.

ethomson · 2018-03-01T09:43:37Z

script/citest.sh

+	ctest -V -R libgit2_clar-proxy_credentials || exit $?
+else
+	echo "Cannot find java in path, skipping proxy test"
+fi


Why are you making this change? Are there instances where travis doesn't have java installed? Or is this for running locally? If the latter, then let's make it some explicit opt-out.

Otherwise I fear that travis will change its images without us noticing, we'll stop running these tests silently, and then we'll break something without knowing.

Yeah, this is slightly conflated with being able to run those scripts in a VM, hence this change (I didn't have java at first).

I understand the concern though, but on the other hand, changing that test made the proxy test run on the osx machines too. If I find a way to tell if the script is Travis run, I'll be able to make it hard error.

ethomson · 2018-03-01T09:44:21Z

script/cileaks.sh

@@ -9,5 +9,5 @@ then
 fi

 if [ -n "$VALGRIND" -a -e "$(which valgrind)" ]; then
-	valgrind --leak-check=full --show-reachable=yes --error-exitcode=125 --suppressions=./libgit2_clar.supp $@ _build/libgit2_clar -ionline
+	valgrind --leak-check=full --show-reachable=yes --error-exitcode=125 --suppressions=./libgit2_clar.supp $@ _build/libgit2_clar -ionline -xbuf::oom


Is this test still useful at all? Maybe we should remove it entirely.

I was originally planning to try and make leaks works too, but I can remove it in the mean time.

ethomson · 2018-03-01T09:46:07Z

script/citest.sh

@@ -10,7 +10,7 @@ fi

 if [ ! -d _build ]; then
 	echo "no _build dir found; you should run cibuild.sh first"
-	exit 1
+	exit 0


I need some more explanation here - why does this fail the coverity build? Again, this seems like something that might could use an explicit flag that the coverity build can pass in so that we don't stop running our build+tests because of some subtle change.

All of the three new "build" steps (cibuild, citest & cileaks) will be run in order now, hence the need to "fail" successfully those 2 new steps so we don't. Alternatively, I could make the coverity.sh explicit instead of calling that from cibuild.sh…

tiennou · 2018-03-02T01:27:55Z

I've removed all changes caused by my trying to reuse the scripts inside VMs. I've also run travis lint locally (hence the suppression of dist).

I've noticed that Travis has build stages, which can accept conditions, so maybe there's a way to make the Coverity/Valgrind scans be less "hacky": right now COVERITY completely shortcuts the build, and VALGRIND will run the test suite twice (once as part of cibuild.sh, and again as part of cileaks.sh).

As this is likely to end up in a mess of pushes, and I can't control AppVeyor builds, I favored being cautious.

pks-t

Thanks for this PR. It helps readability quite a lot to split out those separate scripts

pks-t · 2018-03-26T09:20:17Z

script/cileaks.sh

+	exit $?;
+fi
+
+if [ -n "$VALGRIND" -a -e "$(which valgrind)" ]; then


I'd say we should error out if we request a Valgrind build without Valgrind being installed. Otherwise, we'll get a green CI even if the job doesn't do what it's supposed to do

pks-t · 2018-03-26T09:21:12Z

script/cileaks.sh

+# If this platform doesn't support test execution, bail out now
+if [ -n "$SKIP_TESTS" ];
+then
+	exit $?;


Why do you pass back the value of the previous test? Shouldn't this just exit success?

This is a vanilla copy-paste of the block just above, which appeared in 8f426d7. I honestly have no idea why it's written that way, but it seems to predate AppVeyor, so there's that.

True. Let's leave it as-is, then. But I'd be grateful if you at least remove these useless semicolons ;)

pks-t · 2018-03-26T09:21:57Z

script/cileaks.sh

@@ -9,5 +9,5 @@ then
 fi

 if [ -n "$VALGRIND" -a -e "$(which valgrind)" ]; then
-	valgrind --leak-check=full --show-reachable=yes --error-exitcode=125 --suppressions=./libgit2_clar.supp _build/libgit2_clar -ionline
+	valgrind --leak-check=full --show-reachable=yes --error-exitcode=125 --suppressions=./libgit2_clar.supp $@ _build/libgit2_clar -ionline


This change looks unintentional?

It's intentional, the purpose is that you can VALGRIND=1 ./script/cileaks.sh -stestcase in a VM and get just that valgrinded (note that I actually changed the location of the %@, this one is outdated).

pks-t · 2018-03-26T09:22:35Z

libgit2_clar.supp

+	Memcheck:Leak
+	...
+	fun:curl_global_init
+}


Oh, cool that we can do this now with our global curl initialization.

pks-t · 2018-03-26T09:24:19Z

.travis.yml

@@ -48,13 +48,11 @@ matrix:
   - compiler: gcc
     env: COVERITY=1
     os: linux
-     dist: trusty


I'd rather prefer if we'd delete the global "dist" key. It's nice to have all that information directly in the build matrix.

dist is a global key though, so I can't really use the other. Else I'd have kept the other one 😉.

I don't think this is true. For some time, our matrix contained jobs for both Trusty and Precise, and it worked perfectly fine

tiennou · 2018-04-18T17:27:36Z

Rebased. I yanked the dist: key change out.

tiennou · 2018-04-18T23:18:47Z

🎶 Please stand by while the leaks are getting plugged, and the coverity scans appeased 🎶

tiennou · 2018-04-19T21:07:47Z

This is ready !

tiennou · 2018-04-19T21:37:25Z

I gave a spin to Travis build stages, and it looks like this. Note that the Coverity build doesn't show up since it's not coming from libgit2/libgit2.

I think this could be used to do the pipelining @pks-t envisions in libgit2/discussions#1.

pks-t

Looks all good to me. I'd love if you'd split out the "real" fixes into a separate PR such that we can fast-track them in case this PR here doesn't get merged fast, as I'd like to include them for #4632. Would also make handling on the backport-label a bit easier for me.

pks-t · 2018-04-20T10:45:57Z

src/transports/local.c

 		return 0;
+	}


Would you care to put this into an extra PR? It's worthy to be included in v0.27.1, as I've just introduced that leak with v0.27.0

pks-t · 2018-04-20T10:46:26Z

src/worktree.c

@@ -131,7 +131,7 @@ static int open_worktree_dir(git_worktree **out, const char *parent, const char
 		goto out;
 	}

-	if ((wt = git__calloc(1, sizeof(struct git_repository))) == NULL) {
+	if ((wt = git__calloc(1, sizeof(*wt))) == NULL) {


This one, as well

pks-t · 2018-04-20T10:47:05Z

tests/worktree/worktree.c

@@ -300,6 +300,7 @@ void test_worktree_worktree__add_with_explicit_branch(void)
 	git_reference_free(branch);
 	git_reference_free(wthead);
 	git_repository_free(wtrepo);
+	git_worktree_free(wt);


tiennou · 2018-04-20T17:23:22Z

Moved the reported leaks to #4635 & #4636.

pks-t · 2018-04-30T11:54:00Z

The PR work as advertised ;) Can't merge it due to that, unfortunately, as we got new leaks in our submodule code

tiennou · 2018-05-09T14:32:40Z

I've locally tested that #4641 fixes the leaks (at least from Valgrind's POV), so this depends on that.
The crux of the issue is that because of the duplicated submodule, we're not freeing the first name the second time around.

pks-t · 2018-06-06T06:37:47Z

#4641 is merged now. Could you please rebase on latest master?

==18109== 664 bytes in 1 blocks are still reachable in loss record 279 of 339 ==18109== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==18109== by 0x675B120: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==18109== by 0x675C13C: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==18109== by 0x675C296: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==18109== by 0x679BD14: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==18109== by 0x679CC64: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==18109== by 0x6A64946: ??? (in /usr/lib/x86_64-linux-gnu/libgnutls.so.26.22.6) ==18109== by 0x6A116E8: ??? (in /usr/lib/x86_64-linux-gnu/libgnutls.so.26.22.6) ==18109== by 0x6A01114: gnutls_global_init (in /usr/lib/x86_64-linux-gnu/libgnutls.so.26.22.6) ==18109== by 0x52A6C78: ??? (in /usr/lib/x86_64-linux-gnu/libcurl-gnutls.so.4.3.0) ==18109== by 0x5285ADC: curl_global_init (in /usr/lib/x86_64-linux-gnu/libcurl-gnutls.so.4.3.0) ==18109== by 0x663524: git_curl_stream_global_init (curl.c:44)

==2957== 912 bytes in 19 blocks are still reachable in loss record 323 of 369 ==2957== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2957== by 0x675B120: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x675BDF8: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x675FE0D: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x6761DC4: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x676477E: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x675B071: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x675B544: ??? (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x675914B: gcry_control (in /lib/x86_64-linux-gnu/libgcrypt.so.11.8.2) ==2957== by 0x5D30EC9: libssh2_init (in /usr/lib/x86_64-linux-gnu/libssh2.so.1.0.1) ==2957== by 0x66BCCD: git_transport_ssh_global_init (ssh.c:910) ==2957== by 0x616443: init_common (global.c:65)

==17851== Invalid free() / delete / delete[] / realloc() ==17851== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==17851== by 0x60BBE2B: __libc_freeres (in /lib/x86_64-linux-gnu/libc-2.19.so) ==17851== by 0x4A256BC: _vgnU_freeres (in /usr/lib/valgrind/vgpreload_core-amd64-linux.so) ==17851== by 0x5F8F16A: __run_exit_handlers (exit.c:97) ==17851== by 0x5F8F1F4: exit (exit.c:104) ==17851== by 0x5F74F4B: (below main) (libc-start.c:321) ==17851== Address 0x63153c0 is 0 bytes inside data symbol "noai6ai_cached"

The goal is to let cmake manage the parallelism

tiennou · 2018-06-06T11:16:37Z

Rebased!

pks-t · 2018-06-07T10:31:20Z

Cool, we can finally merge this! Thanks for your awesome work

tiennou force-pushed the travis/war-on-leaks branch from 63a1e2f to c9d58bc Compare February 28, 2018 21:34

ethomson reviewed Mar 1, 2018

View reviewed changes

tiennou force-pushed the travis/war-on-leaks branch from 0787701 to e0581fa Compare March 2, 2018 01:18

pks-t requested changes Mar 26, 2018

View reviewed changes

pks-t added the feedback provided label Mar 26, 2018

tiennou force-pushed the travis/war-on-leaks branch from e0581fa to 5deecc5 Compare April 18, 2018 17:26

tiennou force-pushed the travis/war-on-leaks branch 2 times, most recently from 2dd99c0 to 4d37949 Compare April 18, 2018 23:16

tiennou force-pushed the travis/war-on-leaks branch 2 times, most recently from 2cba134 to a88db36 Compare April 19, 2018 19:36

pks-t approved these changes Apr 20, 2018

View reviewed changes

tiennou force-pushed the travis/war-on-leaks branch from aa65a48 to 55abdd4 Compare April 20, 2018 17:21

tiennou force-pushed the travis/war-on-leaks branch from 55abdd4 to 34aa29d Compare April 20, 2018 21:11

pks-t mentioned this pull request May 9, 2018

WIP: travis: fail on memory leaks #4416

Closed

tiennou added 7 commits June 6, 2018 13:15

travis: split testing from building

2f4e7cb

travis: split valgrind check in its own script

74b0a43

valgrind: skip buf::oom test

573c408

travis: let cmake perform the build & install step

1f4ada2

The goal is to let cmake manage the parallelism

tiennou added 4 commits June 6, 2018 13:15

valgrind: bump num-callers to 50 for fuller stack traces

0fb8c1d

scripts: use leaks on macOS

4c96961

scripts: remove extraneous semicolons

149790b

travis: enable -Werror in the script instead of using the matrix

61eaaad

tiennou force-pushed the travis/war-on-leaks branch from 34aa29d to 61eaaad Compare June 6, 2018 11:16

pks-t merged commit 534b70a into libgit2:master Jun 7, 2018

tiennou deleted the travis/war-on-leaks branch June 11, 2018 18:23

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

travis: war on leaks #4558

travis: war on leaks #4558

tiennou commented Feb 28, 2018 •

edited

ethomson Mar 1, 2018

tiennou Mar 1, 2018

ethomson Mar 1, 2018

tiennou Mar 1, 2018

ethomson Mar 1, 2018

tiennou Mar 1, 2018

tiennou commented Mar 2, 2018

pks-t left a comment

pks-t Mar 26, 2018

pks-t Mar 26, 2018

tiennou Mar 26, 2018

pks-t Mar 27, 2018

pks-t Mar 26, 2018

tiennou Apr 19, 2018

pks-t Mar 26, 2018

pks-t Mar 26, 2018

tiennou Mar 26, 2018

pks-t Mar 27, 2018

tiennou commented Apr 18, 2018

tiennou commented Apr 18, 2018

tiennou commented Apr 19, 2018

tiennou commented Apr 19, 2018

pks-t left a comment

pks-t Apr 20, 2018

pks-t Apr 20, 2018

pks-t Apr 20, 2018

tiennou commented Apr 20, 2018

pks-t commented Apr 30, 2018

tiennou commented May 9, 2018

pks-t commented Jun 6, 2018

tiennou commented Jun 6, 2018

pks-t commented Jun 7, 2018

travis: war on leaks #4558

travis: war on leaks #4558

Conversation

tiennou commented Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Mar 2, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Apr 18, 2018

tiennou commented Apr 18, 2018

tiennou commented Apr 19, 2018

tiennou commented Apr 19, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Apr 20, 2018

pks-t commented Apr 30, 2018

tiennou commented May 9, 2018

pks-t commented Jun 6, 2018

tiennou commented Jun 6, 2018

pks-t commented Jun 7, 2018

tiennou commented Feb 28, 2018 •

edited