Actix-X safety dance - Githubissues

rust-secure-code / safety-dance

Auditing crates for unsafe code which can be safely replaced

Apache License 2.0

532 stars 10 forks source link

Actix-X safety dance #65

Open Dowwie opened 4 years ago

Dowwie commented 4 years ago

Is anyone up for an unsafe challenge? It is my understanding that the people around these parts are capable of taming the unsafe beast in Rust code. I wanted to share with you an opportunity to finally address long-running concerns about unsafe code in the actix projects. Contributors want to get rid of these unsafe. It is a priority. We are all in agreement that it is an issue. Now, contributors are resolving whatever they can, and trying to figure out how to do so as if performing a minimally invasive surgical procedure. One of the more popular unsafe is receiving attention now: https://github.com/actix/actix-net/pull/98 With this benchmarking PR, people can now understand the impact that refactoring will have. If this interests anyone, please feel free to get involved!

tarcieri commented 4 years ago

Shnatsel commented 4 years ago

A few pull requests with safety improvements have landed recently and should ship in version 3.0:

https://github.com/actix/actix-net/pull/158 https://github.com/actix/actix-net/pull/161 https://github.com/actix/actix-web/pull/1614 https://github.com/actix/actix-net/issues/91

However, some unsafe code remains and still needs to be audited for soundness.

alex commented 4 years ago

@Shnatsel do any of these rise to the level of needing an entry in the vuln db?

Shnatsel commented 4 years ago

Unsound Cell impls allow multiple mutable references to the same data, which may lead to pretty much arbitrary memory corruption, including use-after-free. However, it requires somewhat contrived code on the caller side. I'd say it deserves some kind of entry, but I'm not sure whether it should be for a warning or a hard error.

I admit I cannot follow the other two well enough to judge.

Shnatsel commented 3 years ago

actix-web 3.0.0 with notable safety fixes has been released today.

Here's a list of the actix-* crates that still use unsafe code:

actix-http: 13 unsafe blocks, all are commented and look reasonable at a glance. (Some of the benchmarking code looks sketchy, but who cares - it's not in the build anyway). I think use of bytemuck could eliminate at least one unsafe fn in there.
actix-utils: 9 unsafe blocks, no comments on why they're sound. Judging by this comment from one of the Actix org members, a PR with comments explaining why they're sound and/or debug assertions would be appreciated.
actix-router: 1 unsafe block, commented
actix-codec: cargo-geiger shows 10 unsafe expressions but I can't see them in actix git, might be a bug
actix-service: some unsafe code, but cargo-geiger reports that it's not used in the build (likely disabled by a feature)
awc: one unsafe fn without any local uses

So the most valuable audit target seems to be actix-utils

danielhenrymantilla commented 3 years ago

cargo-geiger shows 10 unsafe expressions but I can't see them in actix git, might be a bug

I've run it and for me it shows them for cargo-geiger 0.2.0, not 0.3.0

paulocsanz commented 3 years ago

It seems most bugs 3.0 went through before release were related to mem-leaks, which are not memory errors and can be triggered with safe code. Buuut it seems like a great start to understanding how even a community that's heavily focused on safety can have problems with mem-leaks and maybe we eventually can understand better how to prevent them with cargo tools.

Of course it's a hard task, but it's a pattern that seems to be on the rise. No UB, but struggling with mem-leaks.

Shnatsel commented 3 years ago

Versions in the latest git have even less unsafe code than the 3.0 release