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

Audit async-task #57

Closed
skade opened this issue Dec 10, 2019 · 6 comments
Closed

Audit async-task #57

skade opened this issue Dec 10, 2019 · 6 comments

Comments

@skade
Copy link

skade commented Dec 10, 2019

GitHub location: https://github.com/async-rs/async-task/

async-task is the task allocator of async-std. Tasks are very heavy on raw VTables, so some unsafe code is required. It's roughly 800 lines of code, but has 37 uses of unsafe. Many are pointer to pointer casts.

It's tested and checked with valgrind.

Its only dependency is crossbeam_utils::Backoff, which has no use of unsafe.

@Shnatsel
Copy link
Member

Running Clippy is a good start - it can automatically find some unsound pointer casts.

@Lokathor
Copy link
Contributor

Isn't that newish? How new a clippy does one need to catch that?

@Shnatsel
Copy link
Member

Shnatsel commented Dec 10, 2019

At least one helpful lint has been around for a while - committed to git 2 years ago: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_ptr_to_ptr

But lints implemented based on results of safety-dance are very recent and are not included in stable releases yet, so you should run cargo +nightly clippy

@Lokathor
Copy link
Contributor

My point was exactly that: I only upgrade Nightly when a new Stable comes out. I just run rustup for everything once every 6 weeks and otherwise don't touch it. I suspect many others do the same, even if they use Nightly for various unstable features.

@skade
Copy link
Author

skade commented Dec 11, 2019

We found clippy rather unhelpful there. We actually ended up sending this patch: rust-lang/rust-clippy#4257

The remaining warning is this:

error: casting from `*const u8` to a more-strictly-aligned pointer (`*const header::Header`) (1 < 8 bytes)
   --> src/raw.rs:160:25
    |
160 |                 header: p as *const Header,

in the following code:

    pub(crate) fn from_ptr(ptr: *const ()) -> Self {
        let task_layout = Self::task_layout();
        let p = ptr as *const u8;

        unsafe {
            Self {
                header: p as *const Header,
                tag: p.add(task_layout.offset_t) as *mut T,
                schedule: p.add(task_layout.offset_s) as *const S,
                future: p.add(task_layout.offset_f) as *mut F,
                output: p.add(task_layout.offset_r) as *mut R,
            }
        }
    }

There, as we never dereference the pointer and, I see no issues with keeping this warning over making the code more complex. We need to cast to u8 for proper calculations.

I'll try nightly.

@Shnatsel
Copy link
Member

Shnatsel commented Apr 4, 2020

async-task is also used by the upcoming dramatically simpler runtime, smol.

@skade skade closed this as completed Mar 30, 2021
@8573 8573 mentioned this issue Sep 25, 2023
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