Document or remove some uses of unsafe

[joshlf]

Hey @briansmith, I'm happy to have this merged if it's in a state you're comfortable with, but I also know you were hesitant about taking another dependency. I'd be happy to make any changes or chat about the overall approach.

I can also remove the derive feature from zerocopy if you'd prefer; that's the bulk of the compilation time (zerocopy itself without the derive is a fairly small library). I'm not sure if compilation time is your primary concern, but I know it is for some crate authors.

[codecov]

Codecov Report

Merging #1657 (4056fb9) into main (7bbc307) will increase coverage by 0.00%.
Report is 380 commits behind head on main.
The diff coverage is 98.00%.

@@           Coverage Diff           @@
##             main    #1657   +/-   ##
=======================================
  Coverage   92.09%   92.09%           
=======================================
  Files         132      132           
  Lines       18869    18915   +46     
  Branches      196      196           
=======================================
+ Hits        17377    17420   +43     
- Misses       1455     1458    +3     
  Partials       37       37           

Files	Coverage Δ
src/aead/gcm.rs	77.01% <100.00%> (+0.82%)	⬆️
src/digest.rs	97.36% <100.00%> (+0.33%)	⬆️
src/rsa/public_exponent.rs	98.24% <ø> (ø)
src/endian.rs	90.69% <92.30%> (+3.19%)	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[briansmith]

@ctz @cpu @djc Do you have any worries about adding zerocopy as a dependency or increasing the MSRV to 1.61? I personally think it is important for cleaning up some unsafe stuff in ring that we do increase the MSRV to 1.61 in 0.17.

[briansmith]

In general I am open to doing this but I think if we're going to use zerocopy we should really get more use out of it than this.

I will follow up on this when I am done with the BoringSSL merge.

[briansmith]

Remove the TODO above this.

[joshlf]

I can do that, but it might be good to keep it since .unwrap() would still be preferable to a match. Lmk either way.

[briansmith]

Ditto: Remove the TODO.

[briansmith]

I think a lot of what's in endian.rs likely duplicates the zerocopy functionality that is being derived here. I suspect we could get rid of even more of the unsafe by removing that duplication.

[joshlf]

Good idea. We've got the byteorder module which provides analogous types, although they don't provide alignment guarantees, which can pessimize code.

We've got an open issue to support versions of those types with platform-native alignment, but I haven't had time to work on it yet. If that's the thing that would unblock this, I'd be happy to prioritize it.

[djc]

MSRV: I think bumping the MSRV to 1.61 right now would be fine, but it would be good to understand the MSRV policy going forward. Ideally in other projects I've wanted to stick to a Tokio-like policy of at least 6 months, but also generally be fairly conservative unless there are major benefits to be had (beyond small syntactic conveniences).

I suppose the main qualm I have here is the dependencies that would be brought in. Right now rustls has fairly minimal dependencies:

rustls v0.22.0-alpha.3 (/Users/djc/src/rustls/rustls)
├── log v0.4.20
├── ring v0.16.20
│   └── untrusted v0.7.1
│   [build-dependencies]
│   └── cc v1.0.83
│       └── libc v0.2.148
├── rustls-pki-types v0.2.1
├── rustls-webpki v0.102.0-alpha.3
│   ├── ring v0.16.20 (*)
│   ├── rustls-pki-types v0.2.1
│   └── untrusted v0.7.1 (also from ring)
└── subtle v2.5.0

Bringing in the macros from zerocopy-derive would add syn, quote, and proc-macro2. On the one hand, that's quite a bit of code to compile -- on the other hand, they're bound to be part of downstream's dependency graphs already. @joshlf have you ever contemplated writing some token stream processors without syn and quote? This seems like the kind of dependency where it might be worth it.

@joshlf it looks like you were active within the Safe Transmute project. What happened with that? It seems to have petered out, why? This really seems like something that core could/should provide.

[joshlf]

Bringing in the macros from zerocopy-derive would add syn, quote, and proc-macro2. On the one hand, that's quite a bit of code to compile -- on the other hand, they're bound to be part of downstream's dependency graphs already. @joshlf have you ever contemplated writing some token stream processors without syn and quote? This seems like the kind of dependency where it might be worth it.

zerocopy's derive feature is optional. I enable it in this PR so I can derive some traits on the endinanness types. If, as Brian suggested, I instead removes those types entirely and replace them with the equivalent types in zerocopy, I can disable derive. I'd need to enable byteorder in its place, which would bring in a dependency on the byteorder crate. That crate has no dependencies. If even that crate is considered problematic, I could probably do the work to make it an optional dependency as well (we use some of its types in our byteorder module, but we could just redefine those types without much hassle).

@joshlf it looks like you were active within the Safe Transmute project. What happened with that? It seems to have petered out, why? This really seems like something that core could/should provide.

It's still in progress. There haven't been a ton of development cycles available to spend on it, but there have been some. I wouldn't expect it to land soon, but I would still expect it to land.

[briansmith]

Unfortunately, I made a foobar with this PR and merged it unintendedly as-is. I reverted the merge in #1661. (I meant to merge the latest BoringSSL merge.)

In any case, I think we need to try to using proc_macros and adding syn, etc. as dependencies. I am open to the idea of taking byteorder and related as dependencies but I would prefer to not do it right now until we have a clearer story for how we're going to manage dependencies w.r.t. the FIPS validation. So I think if we could avoid adding byteorder as a dependency and avoid using proc macros, that would be good in the short term.

I would like to split this PR up into multiple ones that address the individual issues:

• Is the use of union in Digest problematic, or just hard to review? This should be a separate issue or PR.
• Use of new_unchecked: Great, let's get rid of it. No brainer.
• What to do with the endian module. Let's have a separate PR/issue for it. Changing it has performance and other considerations.
• Updating the MSRV to 1.61: I will do it.
• MSRV policy going forward: I filed issue Document MSRV Policy #1660 to discuss it.

[joshlf]

• Use of new_unchecked: Great, let's get rid of it. No brainer.

Submitted #1664

Also submitted one safety comment in #1665

• Is the use of union in Digest problematic, or just hard to review? This should be a separate issue or PR.

As best I can tell, it's just hard to review. I took a stab at cleaning it up, but I realized that Digest and Output have to be unions because that's what they are in C, and so to make them structs would be unsound (C could initialize only some of the bytes, which is sound in C since C views it as a union, and then expose uninitialized bytes to Rust). Doing FFI in a way that's easy to review is really hard and takes a lot of extra machinery. We tried to do this in Mundane, and it required a lot of up-front work. IMO it was worth it in the long-run, but it's not the sort of thing that'd make sense to introduce to get rid of a few lines of unsafe. I can take a stab at cleaning up the Digest/Output stuff, but I doubt there'll be much I can do without much deeper surgery.

[briansmith]

MO it was worth it in the long-run, but it's not the sort of thing that'd make sense to introduce to get rid of a few lines of unsafe. I can take a stab at cleaning up the Digest/Output stuff, but I doubt there'll be much I can do without much deeper surgery.

Let's have a chat first. I am planning to do "deeper surgery" anyway to make all these union and union- like things much clearer throughout the codebase. Briefly: we trying to model enums where each variant implements a particular trait. This allows us to simulate support for returning dyn Trait which still isn't supported by Rust yet. Also, we don't actually model the traits as traits, but as Algorithm types, so that we can reify them as objects at runtime, to allow not just compile-time but also runtime selection of which trait implementation one wants to use, to avoid "template bloat". In the future, these are likely to be modeled as traits and we'll introduce some machinery explicitly for "unions of things that implement the same trait, without discriminants in front, and where trait implementations are reified as objects." This is part of the planned API expansion/redesign of ring that I haven't documented yet.