travis: war on leaks by tiennou · Pull Request #4558 · libgit2/libgit2

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

+	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


 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

 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

+	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

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


 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

+	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

   - 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

+	if (git_reference_type(reference) != GIT_REF_OID) {
+		git_reference_free(reference);
 		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

 	}

-	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

 	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

Conversation

tiennou commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Mar 2, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Apr 18, 2018

Uh oh!

tiennou commented Apr 18, 2018

Uh oh!

tiennou commented Apr 19, 2018

Uh oh!

tiennou commented Apr 19, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Apr 20, 2018

Uh oh!

pks-t commented Apr 30, 2018

Uh oh!

tiennou commented May 9, 2018

Uh oh!

pks-t commented Jun 6, 2018

Uh oh!

tiennou commented Jun 6, 2018

Uh oh!

pks-t commented Jun 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tiennou commented Feb 28, 2018 •

edited

Loading