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

chore: remove all unsafe #169

Closed
wants to merge 1 commit into from
Closed

Conversation

wucke13
Copy link
Contributor

@wucke13 wucke13 commented Jul 10, 2024

This commit removes all unsafe (entirely related to transmuting) from the code. Instead, Google's zerocopy crate is used to do the transmuting. Being widely used and thoroughly vetted, it is less likely to cause hidden soundness issues.

The impact to the dependencies is minimal, i. e. the only extra crates added to the Cargo.lock are byteorder, zerocopy and zerocopy-derive.

@wucke13
Copy link
Contributor Author

wucke13 commented Jul 10, 2024

@nspin I would really appreciate your opinion on this

@Ivan-Velickovic
Copy link
Collaborator

Is this fixing a known bug?

The biggest motivation for re-writing the tool from Python to Rust was to avoid dependencies. Because of how few struct_to_bytes and bytes_to_structs calls there are, I would prefer to just do the conversion manually byte by byte for each struct instead of adding dependencies. Then it would be 'safe' and we would not have more dependencies.

@wucke13
Copy link
Contributor Author

wucke13 commented Jul 11, 2024

Is this fixing a known bug?

None that I'm aware of.

Manual conversion byte by byte is fine! There is one occurrence in the code where a mutable ref to a struct is mutated, thus in-place editing the byte slice, that part may need refactoring a little. Doing it by hand will add some more code for sure. It might be sensible to use a macro_rules macro to generate both the to_bytes/from_bytes functions. It of course is the way for the least amount of unsafe in the entire dependencies.

I understand the general wariness towards dependencies, however, if one where to compare different libs, then zerocopy most definitely would fall in the "okay" area. Is super widely used (fourth in popularity for embedded according to lib.rs), it is backed by a big company (Google), almost no transitive dependencies (except for the obvious macro gang proc-macro2, quote and syn), well documented, ...

Avoiding dependencies sounds like a requirement which itself is traceable to other requirement(s). Is there any documentation/public discussion about this? I'd like to understand the rationale behind the requirement.

This commit removes all unsafe (entirely related to transmuting) from
the code. Instead, Google's zerocopy crate is used to do the
transmuting. Being widely used and thoroughly vetted, it is less likely
to cause hidden soundness issues.

Signed-off-by: wucke13 <[email protected]>
@lsf37
Copy link
Member

lsf37 commented Jul 11, 2024

I understand the general wariness towards dependencies, however, if one where to compare different libs, then zerocopy most definitely would fall in the "okay" area. Is super widely used (fourth in popularity for embedded according to lib.rs), it is backed by a big company (Google), almost no transitive dependencies (except for the obvious macro gang proc-macro2, quote and syn), well documented, ..

Just my 2 cents, but my personal experience with this kind of dependency (esp from Google in the Java ecosystem) has been abysmal. They can replace things you can easily do yourself with opaque complex code that does much more than you need and you now have the problem of constantly updating those dependencies when something that might be unrelated to your code is getting fixed upstream.

I'm not saying using libraries is bad, but the tradeoffs really need to be considered deeply and case by case. E.g. if we were doing lots of networking stuff here, I think the library would be great. For these controlled simple cases, probably less so.

(I also do agree with reducing unsafe, of course, not saying we shouldn't do that.)

@Ivan-Velickovic
Copy link
Collaborator

I agree with what Gerwin has said.

Irrespective of the programming language, I find dependencies in general quite dangerous for the longevity and reliability of software, which means we have to evaluate each one we want to add on a case by case basis.

In this case, I would say to replace all the unsafe with manually converting bytes would be 100-200 lines of code. The code itself would be fairly straightfoward I imagine. The zerocopy package would increase our trusted-computing-base by 9,000 lines which makes it not worthwhile in my opinion.

An example where it is worthwhile is the current dependencies of the Microkit tool (an XML parser, and a JSON parser). These would be non-trivial to write ourselves and would most likely have more correctness issues than the dependencies.

I have to push back on adding more dependencies, otherwise I fear the situation will quickly get out of hand.

Avoiding dependencies sounds like a requirement which itself is traceable to other requirement(s). Is there any documentation/public discussion about this? I'd like to understand the rationale behind the requirement.

There is not to my knowledge. I don't think the seL4 Foundation has anything official, but I could add something about the principles of Microkit in #49. It would be good to have something to point to in the future if this topic comes up again.

@wucke13
Copy link
Contributor Author

wucke13 commented Jul 12, 2024

Thank you two for the elaborate explanations! This is quite funny, on the one hand I'm bitten exactly by what you describe with the Nix packaging of the capDL-tool, which subscribes to very specific versions of dependencies (MissingH, ...) which were updated in the nixpkgs, and overriding dependency versions is really sub-optimal in for Haskell in Nix. So I'm basically stuck at an old version of the nixpkgs checkout if I want to be able to compile capDL-tool, which I have to to be able to build the CAmkES VMM examples. So I know exactly the kind of pain you describe, and it is indirectly (over Nix forcing newer versions for Haskell deps) brought to me by you 😆 .

On the other hand, in my past fiver-or-so years of Rust, this was seldomly a pain. Of course there are poorly maintained dependencies that change a lot in the outskirts of crates.io, but I never ran into noteworthy issues with libraries that are well maintained yet limited in scope (such as zerocopy). The only painful experiences were big crates with a large scope that were quite visible 0.x releases while having a large and complex API (the matrix sdk comes to mind 🙄 ). And the clap command line argument parser, which gets rapidly updated and breaks API with every major release (of which there many). In general, Rust folks take semver rather serious, and when a version gets yanked due to a CVE or something, most usually a patch release that otherwise is API compatible is pushed quickly.

TL;DR Compared to many other sw ecosystems, dependency hell is much less pronounced when dealing with Rust.

I say this to spark some hope with you guys, that maybe not all dependency usage is hurtful, especially when you oxidize. While I question the excessive extent of dependency avoidance to some degree, I do respect the strive for close to zero dependencies! I think its safe to assume that this PR will not reach consensus w/r/t the approach taken, so I'm closing it. Again, thank you for the discussion!

@wucke13 wucke13 closed this Jul 12, 2024
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

Successfully merging this pull request may close these issues.

3 participants