Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various memleaks discovered by ASAN #3568

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Mar 22, 2021

So due to a ASAN failure in #3548, I tried to run ASAN on TOSS4 and hit all of these other ASAN discovered memleaks when trying to debug my PR's issue (i.e. make check was constantly failing all over the place, never getting to the unit test failure I hit in CI).

I thought these were new ASAN discoveries in TOSS4 over TOSS3, but @grondo reminded me that we turn off leak detection in the CI b/c there are too many and many can't be fixed. But by this point I had already gone down the path of fixing a bunch of these leaks.

I saw no reason to waste these fixes, so here they are and some extras as I continued the exercise. I did find one actual memleak in libjob, so the exercise wasn't a complete waste of time.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

One suggestion: since the "fix memleak in * test" commits don't have any further description, it may be a bit less noise to combine them into one commit and then maybe say something about the the specific ASAN environment that caught them? The one libjob fix can be its own commit and might do well to use the Problem: format.

Or if that just feels like busywork, I'm actually OK with just setting MWP. Your choice.

@chu11
Copy link
Member Author

chu11 commented Mar 24, 2021

re-pushed per @garlick comments above.

@garlick
Copy link
Member

garlick commented Mar 24, 2021

Looks good! Feel freel to set MWP when you're ready.

Using the configure option --enable-sanitizer=address,
many memleaks were found in unit tests.  This commit fixes
many of them.
Problem: In the event of a invalid input error, there is a chance
memory allocated during a json_loads() may leak.

Solution: Call json_decref() when returning from an input error.
@chu11
Copy link
Member Author

chu11 commented Mar 24, 2021

the bionic builder hung (also did in #3569 .. hopefully not a start of a pattern). restarting it

@garlick
Copy link
Member

garlick commented Mar 25, 2021

Hung again. Looks like in this case, everything ran, but t/t2607-job-shell-input.t did not report in.

In #3569 it looked like it stopped during configure.

Hmm, I'll go ahead and restart again.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #3568 (9ef6008) into master (395b180) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3568   +/-   ##
=======================================
  Coverage   82.47%   82.47%           
=======================================
  Files         323      323           
  Lines       48770    48771    +1     
=======================================
+ Hits        40221    40225    +4     
+ Misses       8549     8546    -3     
Impacted Files Coverage Δ
src/common/libjob/job.c 85.65% <100.00%> (+0.05%) ⬆️
src/common/libflux/message.c 84.04% <0.00%> (-0.13%) ⬇️
src/broker/module.c 75.11% <0.00%> (+0.46%) ⬆️
src/modules/job-info/guest_watch.c 76.75% <0.00%> (+0.61%) ⬆️

@mergify mergify bot merged commit a1d1192 into flux-framework:master Mar 25, 2021
@chu11 chu11 deleted the issue3566_toss4_asan branch June 5, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants