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

libflux: disallow message send; prevent segfaults during flux_handle_destroy() #2713

Merged
merged 3 commits into from Feb 5, 2020

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Feb 5, 2020

This PR is split off #2710. It fixes two issues discussed there:

  1. Prevent use of flux_send and its derivatives during handle destruction (e.g. in aux_item destructors). Attempts to send messages when the handle is being destroyed will now return ENOSYS.
  2. Fix potential re-entry into flux_handle_destroy() by adding a destroy_in_progress flag to flux handles.

Also adds a trivial testcase that segfaulted before these fixes.

Problem: Objects registered with a flux_t aux hash that
attempt to send an RPC during their destructors will segfault
due to a potential to enter flux_handle_destroy() recursively.
This happens because adding the handle to future re-increments
the usecount to 1. When the usecount drops to 0 again after
the future is destroyed, then there is nothing preventing
flux_future_destroy() from being called again.

Add a destroy_in_progress flag to flux_t, and set this as a
sentinel to avoid entering flux_future_destroy() more than once
for any given handle.

Fixes flux-framework#2711
Problem: The flux handle aux_item hash opens an opportunity for
code to attempt to send messages during item destructors, which could
cause difficult to diagnose problems or segfaults, and thus should
be prevented.

Disallow message passing (sending messages at least) during flux handle
destruction, and thus all aux hash destructors, by failing flux_send()
if the handles "destroy_in_progress" flag is set.
Test that an rpc during aux item destructor fails reliably, and
that there is no resulting segfault as described in issue flux-framework#2711.
@codecov-io
Copy link

Codecov Report

Merging #2713 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2713      +/-   ##
==========================================
- Coverage   81.38%   81.38%   -0.01%     
==========================================
  Files         248      248              
  Lines       38789    38791       +2     
==========================================
  Hits        31570    31570              
- Misses       7219     7221       +2
Impacted Files Coverage Δ
src/common/libflux/handle.c 83.03% <100%> (-1.5%) ⬇️
src/cmd/flux-exec.c 76.65% <0%> (-0.45%) ⬇️
src/common/libflux/message.c 81.9% <0%> (+0.13%) ⬆️
src/common/libsubprocess/local.c 80.41% <0%> (+0.34%) ⬆️
src/broker/modservice.c 71.42% <0%> (+0.75%) ⬆️
src/modules/job-info/watch.c 72.53% <0%> (+1.55%) ⬆️

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.

Perfecto.

@grondo
Copy link
Contributor Author

grondo commented Feb 5, 2020

alrighty-then, setting merge-when-passing.

@mergify mergify bot merged commit a64c18a into flux-framework:master Feb 5, 2020
@grondo grondo deleted the issue#2711 branch February 5, 2020 21:19
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.

None yet

3 participants