Skip to content

sdexec: fix use after free during teardown #6037

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

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

garlick
Copy link
Member

@garlick garlick commented Jun 6, 2024

Problem: the sdexec module can segfault when a subprocess unit is destroyed as a result of an output channel closing.

The channel output callback destroys the channel, then the channel code continues referencing it after the callback. Add a reference count that is held over the callback and the duration of the function to prevent the segfault.

Fixes #6036

garlick added 2 commits June 6, 2024 09:56
Problem: the sdexec module can segfault when a subprocess unit
is destroyed as a result of an output channel closing.

The channel output callback destroys the channel, then the channel
code continues referencing it after the callback.  Add a reference
count that is held over the callback and the duration of hte function
to prevent the segfault.

Fixes flux-framework#6036
Problem: there is no test coverage that refcounting protects
an sdexec channel from segfaulting if it is destroyed in the
output callback.

Add a unit test.
@garlick
Copy link
Member Author

garlick commented Jun 6, 2024

I also confirmed this addresses the manual reproducer described in the issue.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Nice! LGTM!

@garlick
Copy link
Member Author

garlick commented Jun 6, 2024

Thanks! I'll set MWP.

@mergify mergify bot merged commit 0ddc3d9 into flux-framework:master Jun 6, 2024
34 checks passed
@garlick garlick deleted the issue#6036 branch June 6, 2024 20:54
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.29%. Comparing base (9944231) to head (47459c1).
Report is 459 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6037       +/-   ##
===========================================
+ Coverage   54.42%   83.29%   +28.87%     
===========================================
  Files         471      519       +48     
  Lines       76273    83686     +7413     
===========================================
+ Hits        41515    69710    +28195     
+ Misses      34758    13976    -20782     
Files with missing lines Coverage Δ
src/common/libsdexec/channel.c 82.92% <100.00%> (-2.79%) ⬇️

... and 439 files with indirect coverage changes

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.

sdexec: broker segfault in outbuf_mark_free
2 participants