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

fix use-after-free of Task with futures2 #314

Closed
wants to merge 1 commit into from

Conversation

kpp
Copy link
Contributor

@kpp kpp commented Apr 11, 2018

Code stolen from #254

Any way there is error after #300:

$ cargo test --all --features="unstable-futures"  -j1
   Compiling tokio-threadpool v0.1.2 (file:///home/humbug/tokio/tokio-threadpool)
error[E0560]: struct `task::Task` has no field named `ptr`
  --> tokio-threadpool/src/task/mod.rs:92:16
   |
92 |         Task { ptr: Box::into_raw(inner) }
   |                ^^^ `task::Task` does not have this field
   |
   = note: available fields are: `state`, `next`, `future`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0560`.
error: Could not compile `tokio-threadpool`.

To learn more, run the command again with --verbose.

Also test by @seanmonstar fails:

$ cargo test --all -j1
...
...
test tests::task_drop_counts ... FAILED

failures:

---- tests::task_drop_counts stdout ----
	thread 'tests::task_drop_counts' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `1`', tokio-threadpool/src/lib.rs:95:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    tests::task_drop_counts

@seanmonstar
Copy link
Member

The tests are probably no longer relevant, since Task no longer manages it's ref counts manually, but just uses Arc. They could probably be removed...

@kpp
Copy link
Contributor Author

kpp commented Apr 19, 2018

They could probably be removed...

I love tests. The more the better. So I would be very grateful if you could fix them please.

@seanmonstar
Copy link
Member

I like tests too. But I frankly don't understand the changes myself since I wrote the original patch.

@carllerche
Copy link
Member

Thanks for this. However, the fix probably should be rethought as the threadpool internals changed. I don't even think the futures2 feature flag compiles.

Anyway, I'm going to close this PR in an effort to keep things organized.

@carllerche carllerche closed this May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants