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 color-eyre and its dependencies #76

Open
Shnatsel opened this issue Mar 7, 2022 · 3 comments
Open

Audit color-eyre and its dependencies #76

Shnatsel opened this issue Mar 7, 2022 · 3 comments

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Mar 7, 2022

color-eyre is a popular crate for error handling, with over 1 million downloads, and the eyre itself has over 3 million downloads.

It relies on a surprising amount of unsafe code, even with default-features = false:

0/0        0/0          0/0    0/0     0/0      ❓ └── color-eyre 0.6.1
7/22       218/779      2/6    0/0     2/5      ☢️      ├── backtrace 0.3.64
0/0        11/26        0/0    0/0     0/0      ☢️      │   ├── addr2line 0.17.0
0/0        29/48        1/3    1/1     0/0      ☢️      │   │   ├── gimli 0.26.1
0/0        24/26        0/0    1/1     0/0      ☢️      │   │   ├── object 0.27.1
20/37      1320/2140    0/0    0/0     16/16    ☢️      │   │   │   └── memchr 2.4.1
0/20       12/327       0/2    0/0     2/30     ☢️      │   │   │       └── libc 0.2.119
0/0        0/0          0/0    0/0     0/0      ❓     │   │   └── rustc-demangle 0.1.21
0/0        0/0          0/0    0/0     0/0      ❓     │   ├── cfg-if 1.0.0
0/20       12/327       0/2    0/0     2/30     ☢️      │   ├── libc 0.2.119
0/0        0/0          0/0    0/0     0/0      🔒     │   ├── miniz_oxide 0.4.4
0/0        0/0          0/0    0/0     0/0      🔒     │   │   └── adler 1.0.2
0/0        24/26        0/0    1/1     0/0      ☢️      │   ├── object 0.27.1
0/0        0/0          0/0    0/0     0/0      ❓     │   └── rustc-demangle 0.1.21
10/10      220/220      0/0    0/0     1/1      ☢️      ├── eyre 0.6.7
0/0        0/0          0/0    0/0     0/0      ❓     │   ├── indenter 0.3.3
1/1        74/93        4/6    0/0     2/3      ☢️      │   └── once_cell 1.10.0
0/0        0/0          0/0    0/0     0/0      ❓     ├── indenter 0.3.3
1/1        74/93        4/6    0/0     2/3      ☢️      ├── once_cell 1.10.0
0/0        16/16        0/0    0/0     0/0      ☢️      └── owo-colors 3.2.0

A number of dependencies look like they shouldn't need custom unsafe code - such as owo-colors, object, gimli, as well as eyre itself.

It would be nice to remove unsafe code where reasonable.

I haven't looked at the details yet, but the object and gimli crates might be possible to switch to bytemuck instead of custom code, and owo-colors should not need any unsafe at all.

@WaffleLapkin
Copy link

It's weird that owo-colors has unsafe given this line from the readme:

No allocations, unsafe, or dependencies required because embedded systems deserve to be pretty too uwu.

The only place I've found unsafe in there is used to convert &[u8] to &str in a const. Given that std::str::from_utf8 is const unstable I don't think that there is a simple way to remove this unsafe.

@danielhenrymantilla
Copy link

danielhenrymantilla commented Mar 8, 2022

FWIW, it could use str::from_utf8_unchecked, which would already be a bit less unsafe than a transmute (one oughtn not to transmute repr(Rust) entities, even just the wide-pointer ones).
Also, there could be a smaller const_concat!-like dependency in charge of doing the unsafe to perform the concatenation.

  • In this case, for instance, something like a crate exposing a AsciiU8 const-constructible type, and a [AsciiU8] -> str unsafe-using conversion which would be trivial to review as sound 🤔

@yaahc
Copy link

yaahc commented May 24, 2022

I haven't looked at the details yet, but the object and gimli crates might be possible to switch to bytemuck instead of custom code, and owo-colors should not need any unsafe at all.

This would have to be done in backtrace itself, not as part of color-eyre, also, std::backtrace::Backtrace itself also depends on backtrace-rs12 and I believe enables gimli as well3 so I'd recommend treating that as it's own independent effort rather than as part of a review of color-eyre.

Eyre has a known issue4 and needs a fix and I've had a couple people say they're willing to do the fix but so far that hasn't materialized, so that would be the best starting point imo, and I imagine the lions share of further improvements that could be made would also need to happen in eyre itself rather than any of the other deps. I'm happy to help explain any of the unsafe that is currently present. I imagine the biggest simplification would be to rework the special downcasting support5 we inherited from anyhow. It may also be possible to rip out the custom thin pointer logic we have and replace it with https://doc.rust-lang.org/nightly/std/boxed/struct.ThinBox.html.

Footnotes

  1. https://github.com/rust-lang/rust/tree/master/library

  2. https://github.com/rust-lang/rust/blob/master/library/std/src/lib.rs#L580

  3. https://github.com/rust-lang/rust/blob/master/library/std/Cargo.toml#L52

  4. https://github.com/yaahc/eyre/issues/59

  5. https://docs.rs/eyre/latest/eyre/trait.WrapErr.html#effect-on-downcasting

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

4 participants