Use leak sanitizer instead of internal mdebug to check for memory leaks by levitte · Pull Request #9294 · openssl/openssl

levitte · 2019-07-02T14:26:53Z

The leak sanitizer gives better reports (complete stack traces) and
works as a wrapper around the application instead of relying on
cooperative enabling and disabling calls (which are too easy to get
unbalanced).

Related to #8322

levitte · 2019-07-02T14:27:11Z

This also replaces #8323

mspncp · 2019-07-02T15:36:44Z

fly-by-comment: typo in commit message title (and GitHub title): 'sasnitizer'

richsalz · 2019-07-02T17:45:15Z

Should https://github.com/openssl/tools/blob/master/review-tools/opensslbuild#L36 also be changed?

levitte · 2019-07-02T17:56:13Z

fly-by-comment: typo in commit message title (and GitHub title): 'sasnitizer'

Fixed

I also changed the way it's enabled, as sufficiently old clang doesn't understand -fsanitize=leak, but the address sanitizer has understood to enable the leak sanitizer internally for longer.

levitte · 2019-07-02T17:57:22Z

Should https://github.com/openssl/tools/blob/master/review-tools/opensslbuild#L36 also be changed?

I think it should. The leak sanitizer gives far better output than our mdebug ever will.

mattcaswell · 2019-07-03T10:47:20Z

Travis failure looks relevant:

==34177==AddressSanitizer: detect_leaks is not supported on this platform.

levitte · 2019-07-03T15:15:23Z

Damn...

t-j-h · 2019-07-15T07:40:17Z

Drop that platform out for this check.
It is possible to build a mingw cross compiler with asan support - but it isn't necessary to get the testing we need here for most linux-like platforms.

levitte · 2019-07-15T08:17:00Z

Drop that platform out for this check.

I am worried about corner cases for code paths that only happen on dropped platforms.... on the other hand, since this approach is clang/gcc centric already, maybe I worry too much.

levitte · 2019-10-31T12:10:29Z

I'll rework this to avoid mingw... that will require some major surgery of the build matrix

richsalz · 2019-10-31T13:14:58Z

Are you still intending to drop the internal memory-leak tooling? I totally support that in favor of ASAN. ASAN is what everyone is doing. :)

levitte · 2019-10-31T14:43:22Z

Yup, that's my intention.

(that will be like closing a big circle, considering the mdebug stuff was my first commit as a team member, back almost exactly 20 years ago)

levitte · 2019-10-31T14:45:49Z

BTW, I have no idea why mingw was mentioned at all here. The Travis failure happens on MacOS X. So, er, should we simply not do this on MacOS X?

levitte · 2019-10-31T14:48:30Z

The MacOS problem has been complained about here: google/sanitizers#1026
Apparently, it comes down to clang...

richsalz · 2019-10-31T18:21:54Z

When you remove the mdebug stuff, please also remove all the fault-insertion stuff in crypto/mem.c (shouldfail(), md_trace_fd, etc)

mattcaswell · 2019-11-01T09:53:57Z

I think we should just disable this on MacOS.

levitte · 2019-11-22T14:33:45Z

I solved the MacOS case by not specifying ASAN_OPTIONS="detect_leaks=1" in .travis.yml. Leak detection is on by default in modern enough ASAN, I hope that makes it all reliable enough.

mspncp · 2019-11-22T20:21:55Z

Sorry, I couldn’t resist any longer 😉.

levitte · 2019-11-22T20:31:01Z

That's all right. Thanks

levitte · 2019-11-23T09:41:42Z

I'm pondering if I should add a test program that intentionally leaks memory, for demonstration purposes...

paulidale

I'm inclined to think yes to the deliberately leaking test program.
We'd quickly find out if leak detection ever stopped working.

…ory leaks

Detects if leak sanitizing is on, and directs the exit code accordingly. Note that this program is designed to fail when leaking, as that's expected, so to make it easy for wrapper scripts, we also make it look like it fails when sanitizing isn't on.

levitte · 2019-12-03T23:27:14Z

CIs are happy. Is @openssl happy?

paulidale

I'm okay with this.

test/recipes/90-test_memleak.t

richsalz · 2019-12-04T02:45:25Z

are you going to remove the current mdebug cruft?

levitte · 2019-12-04T06:59:17Z

are you going to remove the current mdebug cruft?

That's my intention, yes.

levitte · 2019-12-04T07:00:49Z

... or... wanna take that on? I wouldn't mind. You do need to retain the public API, though...

richsalz · 2019-12-04T16:01:58Z

I will make a PR to remove the existing mem-debug stuff. I had wanted to remove the malloc-fail stuff as well, but it seems there's interest in this now. (I forget which comment in which PR)

richsalz · 2019-12-04T19:28:23Z

PR #10572

The leak sanitizer gives better reports (complete stack traces) and works as a wrapper around the application instead of relying on cooperative enabling and disabling calls (which are too easy to get unbalanced). Related to #8322 Reviewed-by: Paul Dale <[email protected]> (Merged from #9294)

Detects if leak sanitizing is on, and directs the exit code accordingly. Note that this program is designed to fail when leaking, as that's expected, so to make it easy for wrapper scripts, we also make it look like it fails when sanitizing isn't on. Reviewed-by: Paul Dale <[email protected]> (Merged from #9294)

levitte · 2019-12-10T13:19:10Z

Merged.

22c2236 Use leak sanitizer instead of internal mdebug to check for memory leaks
ea7a952 test/memleaktest.c: Modify for use with address/leak sanitizer

levitte · 2019-12-10T13:19:48Z

@richsalz, you've got a small update to do with test/memleaktest.c in #10572 😉

richsalz · 2019-12-10T16:51:25Z

yes, i rebased, took master for the conflicts, squashed, and pushed. i'll note that over there too.

mattcaswell · 2019-12-23T14:05:05Z

This doesn't actually seem to work in travis. The memleak test is failing in the extended tests.

mattcaswell · 2019-12-23T15:10:32Z

I'm proposing to temporarily revert this in #10689.

levitte added the branch: master Applies to master branch label Jul 2, 2019

levitte force-pushed the mdebug-sanitizer branch from 7afb1c6 to 23b6e49 Compare July 2, 2019 17:54

levitte mentioned this pull request Jul 15, 2019

WIP: Use valgrind instead of internal mdebug to check for memory leaks #8323

Closed

levitte changed the title ~~Use leak sasnitizer instead of internal mdebug to check for memory leaks~~ WIP: Use leak sasnitizer instead of internal mdebug to check for memory leaks Oct 31, 2019

levitte force-pushed the mdebug-sanitizer branch from 23b6e49 to 812e328 Compare November 22, 2019 14:32

mspncp changed the title ~~WIP: Use leak sasnitizer instead of internal mdebug to check for memory leaks~~ WIP: Use leak sanitizer instead of internal mdebug to check for memory leaks Nov 22, 2019

levitte marked this pull request as ready for review November 23, 2019 09:40

levitte changed the title ~~WIP: Use leak sanitizer instead of internal mdebug to check for memory leaks~~ Use leak sanitizer instead of internal mdebug to check for memory leaks Nov 23, 2019

levitte added the approval: review pending This pull request needs review by a committer label Nov 23, 2019

paulidale approved these changes Nov 23, 2019

View reviewed changes

levitte added 4 commits November 29, 2019 10:26

fixup! Use leak sanitizer instead of internal mdebug to check for mem…

11d8197

…ory leaks

fixup! test/memleaktest.c: Modify for use with address/leak sanitizer

9a9cff9

fixup! test/memleaktest.c: Modify for use with address/leak sanitizer

6707412

levitte force-pushed the mdebug-sanitizer branch from 9695195 to 6707412 Compare November 29, 2019 09:27

paulidale approved these changes Dec 4, 2019

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 4, 2019

richsalz reviewed Dec 4, 2019

View reviewed changes

test/recipes/90-test_memleak.t Show resolved Hide resolved

levitte mentioned this pull request Dec 4, 2019

Deprecate most of debug-memory #10572

Closed

levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 10, 2019

levitte closed this Dec 10, 2019

mattcaswell mentioned this pull request Dec 23, 2019

Fix Travis #10689

Closed

richsalz mentioned this pull request Jan 21, 2020

Remove NOTES.VALGRIND? #10921

Closed

Uh oh!

Comments

Conversation

levitte commented Jul 2, 2019

Uh oh!

levitte commented Jul 2, 2019

Uh oh!

mspncp commented Jul 2, 2019

Uh oh!

richsalz commented Jul 2, 2019

Uh oh!

levitte commented Jul 2, 2019

Uh oh!

levitte commented Jul 2, 2019

Uh oh!

mattcaswell commented Jul 3, 2019

Uh oh!

levitte commented Jul 3, 2019

Uh oh!

t-j-h commented Jul 15, 2019

Uh oh!

levitte commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Oct 31, 2019

Uh oh!

richsalz commented Oct 31, 2019

Uh oh!

levitte commented Oct 31, 2019

Uh oh!

levitte commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Oct 31, 2019

Uh oh!

richsalz commented Oct 31, 2019

Uh oh!

mattcaswell commented Nov 1, 2019

Uh oh!

levitte commented Nov 22, 2019

Uh oh!

mspncp commented Nov 22, 2019

Uh oh!

levitte commented Nov 22, 2019

Uh oh!

levitte commented Nov 23, 2019

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

levitte commented Dec 3, 2019

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

richsalz commented Dec 4, 2019

Uh oh!

levitte commented Dec 4, 2019

Uh oh!

levitte commented Dec 4, 2019

Uh oh!

richsalz commented Dec 4, 2019

Uh oh!

richsalz commented Dec 4, 2019

Uh oh!

levitte commented Dec 10, 2019

Uh oh!

levitte commented Dec 10, 2019

Uh oh!

richsalz commented Dec 10, 2019

Uh oh!

mattcaswell commented Dec 23, 2019

Uh oh!

mattcaswell commented Dec 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

levitte commented Jul 15, 2019 •

edited

Loading

levitte commented Oct 31, 2019 •

edited

Loading