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

Change flux_handle_destroy prototype to avoid potential memory corruption, fix small leak in flux cmd help #1067

Merged
merged 2 commits into from May 17, 2017

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented May 17, 2017

As discussed in #1062, we were seeing memory corruption from an error path in ssh connector because of the order of destruction of connector implementation object and flux handle object. Since flux_handle_destroy wanted to nullify its argument, but called the implementation's destroy method first, this could result in writing to freed memory if the handle was stored within the implementation (as in ssh connector).

Long story short - it is probably simplest to avoid the "destroy nullifies its argument" idiom in this case, as it is rarely used in the rest of flux API, and we can thereby avoid other accidental cases of memory corruption like this in the future.

I also found a small memory leak in cmd/cmdhelp.c that I threw in here (after some valgrind runs)

grondo added some commits May 16, 2017

libflux: don't nullify argument to flux_handle_destroy
In the ssh connector, the flux handle is saved as a member of the ssh
connector implementation object. On an error path during initialization,
the cleanup calls flux_handle_destroy as

 flux_handle_destroy (&c->h);

where `c` is the connector object.

The implementation destroy operation (h->ops->impl_destroy()) is called
during flux_handle_destroy(), which frees the memory for `c`. As a final
step, flux_handle_destroy() attempts to nullify its argument (&c->h), which
now points to invalid memory and will possibly cause memory corruption and
a crash.

There are multiple ways to fix this, but the most straightforward is
probably to change the prototype of flux_handle_destroy() to take a
reference to `flux_t` instead of a pointer to a reference, and remove
the attempted nullification of its argument.

Fixes #1062
cmdhelp: fix small leak in command_list_read
Fix missing free of temporary variable in command_list_read
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.022% when pulling 57d6216 on grondo:issue#1062 into 61bae10 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 17, 2017

Codecov Report

Merging #1067 into master will decrease coverage by 0.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   77.78%   77.77%   -0.02%     
==========================================
  Files         148      148              
  Lines       25772    25771       -1     
==========================================
- Hits        20048    20043       -5     
- Misses       5724     5728       +4
Impacted Files Coverage Δ
src/cmd/cmdhelp.c 85.32% <100%> (+0.13%) ⬆️
src/connectors/ssh/ssh.c 85.3% <100%> (ø) ⬆️
src/common/libflux/handle.c 84.41% <72.22%> (-0.66%) ⬇️
src/common/libflux/request.c 89.04% <0%> (-1.37%) ⬇️
src/common/libflux/response.c 83.76% <0%> (-0.86%) ⬇️
src/common/libflux/message.c 81.45% <0%> (-0.12%) ⬇️
src/common/libutil/dirwalk.c 90.71% <0%> (+0.71%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 17, 2017

Thanks for fixing that @grondo. Looks good to me.

@garlick garlick merged commit 583c8ec into flux-framework:master May 17, 2017

2 of 4 checks passed

codecov/patch 75% of diff hit (target 77.78%)
Details
codecov/project 77.77% (-0.02%) compared to 61bae10
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.022%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@grondo grondo deleted the grondo:issue#1062 branch Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.