Fix: Make Metadata Send to avoid unsound behavior #101

ram-nad · 2026-01-06T02:50:13Z

Check #100 for details.

#100 (comment)
#100 (comment)
#100 (comment)

Summary:

Without M: Send bound on metadata, we can have unsound behavior. Specifically, as Waker is always Send + Sync, it can be dropped in any thread and in current implementation, we will drop M as well in the thread where we drop last Waker.

This PR does following changes:

Add M: 'static bound on all the safe APIs, we need this because metadata will need to be valid as long as Runnable or Task is valid.
We make sure that we drop M (metadata) whenever we have dropped both Runnable and Task.
Add M: Send bound for Builder::spawn. This is required to avoid unsound behavior described above. We don't need this bound in Builder::spawn_local because, we require that Runnable be dropped/used on same thread if we use this API. Also Task: !Send if M: !Send. Therefore, because of change (2) we will never drop M on other threads if M: !Send.
We add M: Sync bounds in .metadata() API for both Runnable and Task.

Closes #100

ram-nad · 2026-01-06T03:11:57Z

@notgull Should we change the existing example here?

nurmohammed840 · 2026-01-06T03:32:55Z

Could you please split this into separate smaller pull requests for each change.
Large PRs are difficult to review.

ram-nad · 2026-01-06T03:36:39Z

Most changes in PR are due to (2). I am not sure if we can split it further. Also, (3) is dependent on (2) so we have it this way. I can add comments in the changes tab to specify what is being done if that makes it simpler to review?

ram-nad · 2026-01-06T03:37:21Z

src/header.rs

+    ///
+    /// This metadata will always be manually dropped,
+    /// whenever `Runnable` and `Task` are both dropped.
+    pub(crate) metadata: ManuallyDrop<M>,


We will drop metadata separately from task, so mark it as ManuallyDrop.

ram-nad · 2026-01-06T03:37:56Z

src/raw.rs

    pub(crate) drop_future: unsafe fn(*const (), &TaskLayout),

+    /// Drops the metadata associated with the task.
+    pub(crate) drop_metadata: unsafe fn(*const ()),


This function will be used to drop metadata. This is used when dropping the Task.

ram-nad · 2026-01-06T03:39:15Z

src/raw.rs

                propagate_panic,
            },
-            metadata,
+            metadata: core::mem::ManuallyDrop::new(metadata),


We need to initialize metadata as ManuallyDrop (inside allocate_task)

ram-nad · 2026-01-06T03:40:35Z

src/runnable.rs


-            // Drop the task reference.
-            drop_ref(ptr);
+            drop_runnable::<M>(state, ptr);


We are dropping Runnable here, using the new helper function.

ram-nad · 2026-01-06T03:50:40Z

src/runnable.rs


-unsafe impl<M: Send + Sync> Send for Runnable<M> {}
-unsafe impl<M: Send + Sync> Sync for Runnable<M> {}
+unsafe impl<M> Send for Runnable<M> {}


Update bounds for Runnable. It will always be Send (as Waker: Send). It will be Sync only if M: Sync. Note we don't need M: Send for it to be Sync as we cannot drop metadata using its reference to Runnable.

ram-nad · 2026-01-06T03:51:39Z

src/runnable.rs

    /// Tasks can be created with a metadata object associated with them; by default, this
    /// is a `()` value. See the [`Builder::metadata()`] method for more information.
-    pub fn metadata(&self) -> &M {
+    pub fn metadata(&self) -> &M


If M: !Sync it will be unsafe to have this function as we could access metadata from both Task and Runnable which could be in different threads. So, we need this bound here.

ram-nad · 2026-01-06T03:52:25Z

src/runnable.rs

    /// ```
    pub fn spawn<F, Fut, S>(self, future: F, schedule: S) -> (Runnable<M>, Task<Fut::Output, M>)
    where
+        M: Send + 'static,


Since the Future is Send here, we can poll it from any thread. So, we need M: Send as well.

ram-nad · 2026-01-06T03:52:55Z

src/runnable.rs

        schedule: S,
    ) -> (Runnable<M>, Task<Fut::Output, M>)
    where
+        M: 'static,


This is spawn_local, we don't need Send here as described in the PR.

ram-nad · 2026-01-06T03:53:49Z

src/task.rs

+                                    //
+                                    // Safe to call as metadata wouldn't have been
+                                    // dropped before, because we just removed `Task` reference.
+                                    ((*header).vtable.drop_metadata)(ptr);


We are dropping Task here, therefore, we need to drop metadata as well. (We are sure that Runnable has been dropped at this point)

ram-nad · 2026-01-06T03:54:40Z

src/raw.rs

+/// The task must be closed before this function is called. Also,
+/// we need to make sure that both Runnable and Task are dropped.
+#[inline]
+pub(crate) unsafe fn drop_metadata<M>(ptr: *const ()) {


New function to drop metadata.

ram-nad · 2026-01-06T03:55:46Z

src/raw.rs

+/// This is a helper function that we call whenever we are dropping
+/// task reference (as Runnable) and the task is already completed/closed.
+#[inline]
+pub(crate) unsafe fn drop_runnable<M>(state: usize, ptr: *const ()) {


Helper function to drop Runnable. Every time we drop Runnable, we need to check if Task has already been detached. If it has been then we need to drop metadata as well.

ram-nad added 2 commits January 6, 2026 07:56

Fix: Make Metadata Send to avoid unsound behavior

ddde296

Fix: Add Sync bounds

e349490

ram-nad mentioned this pull request Jan 6, 2026

Fix Send and Sync bounds for M to avoiding unsound behavior. #100

Open

ram-nad commented Jan 6, 2026

View reviewed changes

Fix: Make Metadata Send to avoid unsound behavior #101

Are you sure you want to change the base?

Fix: Make Metadata Send to avoid unsound behavior #101

Uh oh!

Conversation

ram-nad commented Jan 6, 2026

Uh oh!

ram-nad commented Jan 6, 2026

Uh oh!

nurmohammed840 commented Jan 6, 2026

Uh oh!

ram-nad commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants