fix possible use-after-free in chained future implementation #5927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
garlick
approved these changes
May 1, 2024
| @@ -432,6 +442,18 @@ static void chained_future_init (flux_future_t *f, void *arg) | |||
| fulfill_next (f, cf->next); | |||
| } | |||
|
|
|||
| void cf_next_destroy (struct chained_future *cf) | |||
Problem: There is trailing whitespace and other formatting in src/common/libflux/composite_future.c that does not meet current project norms. Remove trailing whitespace. Split long function arguments and conditionals one per line where necessary.
Problem: There is redundant code in chained_future_create() when the constructor is called for the same future twice, e.g. when using flux_future_and_then(3) _and_ flux_future_or_then(3). Just skip the redundant code by returning early if the chained future aux item is already present in the target future.
Problem: If the next future in a chained future is destroyed before the previous future, then a use-after-free can occur if the previous future is eventually fulfilled, since chained_continuation() and/or flux_future_continue(3) reference the now destroyed cf->next. This condition can occur, for example, if a timeout is specified in a call to flux_future_then(3) on cf->next, but can also occur in cases where cf->next is destroyed early for other reasons. Additionally, a reasonable expectation is that when next is destroyed, this prevents prev (and any other previous futures) callbacks from being called, either by destroying them or by other means. Arrange to notify the chained_future object when cf->next is destroyed by using an aux_item destructor. Destroy cf->prev if it is non-NULL. Ensure cf->prev is not subject to double-free by nullifying it when cf->prev is destroyed. This allows cf->prev and cf->next to be destroyed in any order safely. Since cf is associated in the aux_item list of cf->prev, and it is now referenced by cf->next as well, add a refcount to this object and take a reference for both cf->prev and cf->next. When both references are released the object will be destroyed. Fixes flux-framework#5923
Problem: A function in test/composite_future.c is called flux_future_timeout() which is confusing since this is not an exported libflux symbol. Rename the function simply future_timeout().
Problem: No tests in the testsuite ensure that there is not a use-after-free when the next future in a chain is destroyed before the previous if fulfilled. Add a repoducer to the composite_future unit tests.
|
Ok, fixed the non-static function and will set MWP. Thanks! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5927 +/- ##
=======================================
Coverage 83.34% 83.34%
=======================================
Files 514 514
Lines 82974 83001 +27
=======================================
+ Hits 69158 69181 +23
- Misses 13816 13820 +4
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the problem with chained futures described in #5923: if a
nextfuture in a chain is destroyed before theprevfuture, then a segfault due to use-after-free could occur later ifprevis eventually fulfilled since this references next.The case where this is likely to occur is when
flux_future_then(3)is called with a timeout on anextfuture, since the timeout will fulfillnextwith an error without even touching theprevfuture.The fix is not exactly elegant. A reference to the
chained_futureobject is placed in thenextfuture'sauxlist, so that when it is destroyed it can (optionally) destroyprev. However, this needs to be avoided in the common case whereprevis destroyed beforenext, so an aux_item destructor is also placed inprevto nullifycf->prevso that future is not double freed. Finally, since the underlyingchained_futureobject is now referenced in multiple places, and these are not called in any guaranteed order, that structure gets a reference counter and a reference count of 3:prevaux_item list asflux::chainednextaux_item listprevaux_item list to callnullify_prevA test is added and was tested under valgrind etc.