NotificationsYou must be signed in to change notification settings
Fork341
Star4k

Fix double drop in StreamExt::cycle#903

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

yoshuawuyts merged 1 commit intoasync-rs:masterfromtaiki-e:stream-cycle

Nov 5, 2020

Merged

Fix double drop in StreamExt::cycle#903

yoshuawuyts merged 1 commit intoasync-rs:masterfromtaiki-e:stream-cycle

Nov 5, 2020

Conversation

Copy link

Contributor

taiki-e commentedNov 1, 2020

StreamExt::cycle dropsource field without updating field ifsource field's destructor panicked.

async-std/src/stream/stream/cycle.rs

Lines 47 to 48 in11196c8

	ManuallyDrop::drop(&mut this.source);
	this.source =ManuallyDrop::new(this.orig.clone());

This is unsound because it causes double drop if the user catches panic in the scope where theCycle is alive.

Repro:

#[test]#[should_panic(expected ="second")]fntest(){use async_std::stream::*;use futures::task::{noop_waker,Context,Poll};use std::{panic, pin::Pin, thread};#[derive(Clone,Default)]structS{dropped:bool,}implStreamforS{typeItem =();fnpoll_next(self:Pin<&mutSelf>, _:&mutContext<'_>) ->Poll<Option<Self::Item>>{Poll::Ready(None)}}implDropforS{fndrop(&mutself){if !thread::panicking(){if std::mem::replace(&mutself.dropped,true){panic!("second")}else{panic!("first")}}}}letmut s =Box::pin(S::default().cycle());let w =noop_waker();let cx =&mutContext::from_waker(&w);let _ = panic::catch_unwind(panic::AssertUnwindSafe(||{let _ = s.as_mut().poll_next(cx);// <-- dropped the first clone of `s`}));// <-- dropped the first clone of`s` again since `source` field wasn't updated}