rust-secure-code / safety-dance

Auditing crates for unsafe code which can be safely replaced
Apache License 2.0
536 stars 10 forks source link

Audit async-task #57

Closed skade closed 3 years ago

skade commented 4 years ago

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 commented 4 years ago

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

Lokathor commented 4 years ago

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

Shnatsel commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

We found clippy rather unhelpful there. We actually ended up sending this patch: https://github.com/rust-lang/rust-clippy/pull/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 commented 4 years ago

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