-
Notifications
You must be signed in to change notification settings - Fork 633
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 use after free of task in FuturesUnordered when dropped future panics #2886
Fix use after free of task in FuturesUnordered when dropped future panics #2886
Conversation
…reference count release when dropping a future panics and unwinds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes for reviewers
// Use ManuallyDrop to transfer the reference count ownership before | ||
// dropping the future so unwinding won't release the reference count. | ||
let md_slot; | ||
let task = if prev { | ||
md_slot = mem::ManuallyDrop::new(task); | ||
&*md_slot | ||
} else { | ||
&task | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main fix
struct DropGuard<'a, Fut>(&'a mut FuturesUnordered<Fut>); | ||
impl<Fut> DropGuard<'_, Fut> { | ||
fn drop_futures(&mut self) { | ||
// When a `FuturesUnordered` is dropped we want to drop all futures | ||
// associated with it. At the same time though there may be tons of | ||
// wakers flying around which contain `Task<Fut>` references | ||
// inside them. We'll let those naturally get deallocated. | ||
while !self.0.head_all.get_mut().is_null() { | ||
let head = *self.0.head_all.get_mut(); | ||
let task = unsafe { self.0.unlink(head) }; | ||
self.0.release_task(task); | ||
} | ||
} | ||
} | ||
impl<Fut> Drop for DropGuard<'_, Fut> { | ||
fn drop(&mut self) { | ||
self.drop_futures(); | ||
} | ||
} | ||
let mut guard = DropGuard(self); | ||
guard.drop_futures(); | ||
mem::forget(guard); // no need to check head_all again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added to make sure all futures are dropped if one panics. Although it looks like tasks have a check for this in their drop that aborts. At least with this, leaking and aborts may be avoided if only one future panics.
// https://github.com/rust-lang/futures-rs/issues/2863#issuecomment-2219441515 | ||
#[test] | ||
#[should_panic] | ||
fn panic_on_drop_fut() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimal example taken from issue. This failed under miri before the changes to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The fix looks good to me.
// If a second future panics, that will trigger an abort from panicking | ||
// while panicking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I would prefer to leak the remaining elements after the first panic has occurred. That is the same behavior as in the standard library and crossbeam's queues/channels (rust-lang/rust#108164 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can change it to that by leaking a clone of the queue Arc
, it's simpler.
I wasn't sure what would be ideal here so I just went with the behavior of Vec
/struct fields/slices/etc. I don't have a particular preference, but I'm a bit curious about the divergence here. I guess the rationale is to prefer leaking over an abort that stops the whole process? Is there something about queues/channels that makes them special here or is it just that we have the freedom to select this preferred behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm miri is complaining about the leak.
With
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check -Zmiri-disable-isolation" cargo +nightly miri nextest run --workspace --all-features -- panic_on_drop_fut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a separate miri invocation for the test that ignores leaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw there might be some potential to make the miri CI faster using nextest
9dfb0ed
to
67f8869
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Hmm. I see AddressSanitizer also complains about the same leak. Given that it would complicate CI, and how Vec and VecDeque behave, trying to avoid the risk of potential abort for bad future here may not actually be a worthwhile goal... |
I could just ignore this test there? The main thing it tests for is caught by miri. It feels kind of bad to change this due to testing issues.... TBH if there was a test for hitting the abort case that would probably have issues too since iirc there isn't a good way to test |
…ics and avoid potential abort if we try to continue dropping futures and another one panics
67f8869
to
f7464a4
Compare
futures-util 0.3.30 was yanked because it had a use after free, see <rust-lang/futures-rs#2886>.
Fixes #2863