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

Cancellation safety #1

Open
Nugine opened this issue Aug 1, 2021 · 6 comments
Open

Cancellation safety #1

Nugine opened this issue Aug 1, 2021 · 6 comments

Comments

@Nugine
Copy link

Nugine commented Aug 1, 2021

Worker::tag_recv accepts &mut [MaybeUninit<u8>] as a data buffer.

pub async fn tag_recv(&self, tag: u64, buf: &mut [MaybeUninit<u8>]) -> usize {
let (_, len) = self.tag_recv_mask(tag, u64::max_value(), buf).await;
len
}

However, a future can be cancelled(dropped) at any time. While the IO operation is still in progress, the data buffer may be used after free. It's an unsoundness issue.

@wangrunji0408
Copy link
Collaborator

Yes, this issue needs to be fixed in the future.

One possible way is to call ucp_request_cancel in the drop function.

@Nugine
Copy link
Author

Nugine commented Aug 2, 2021

The data buffer is invalid once the drop function is called.
Any IO operation should stop using the buffer immediatiley.
Does ucp_request_cancel have such an effect?

@wangrunji0408
Copy link
Collaborator

I think it does. But I can't find proof in the UCX docs.

@Nugine
Copy link
Author

Nugine commented Aug 2, 2021

This line requires a mutable exclusive access. But the completion callback may be called in another thread at any time.
Is there any guarantee for the &mut Request?

let request = unsafe { &mut *(self.ptr as *mut Request) };

@wangrunji0408
Copy link
Collaborator

Oh in fact an immutable reference is enough, because the AtomicWaker guarantees thread safety.

And it is also safe to get mutable reference here (but don't pass it out). Because both Worker and Endpoint are !Send, IO functions and their callbacks are running on the same thread.

@yiyuanliu
Copy link
Contributor

Yes, this issue needs to be fixed in the future.

One possible way is to call ucp_request_cancel in the drop function.

ucx_request_cancel can't guarantee cancellation safety.

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

No branches or pull requests

3 participants