search

google / fleetspeak-rs

A Fleetspeak connector library for the Rust programming language.
MIT License
7 stars 7 forks source link

Fix safety issues with comms channels (#4) #6

Closed panhania closed 5 months ago

panhania commented 6 months ago

The root of the safety issue from issue #4 is that we instantiate std::fs::File from an integer of uncertain origin. This is problematic because we cannot guarantee with absolute certainty that we are the exclusive owners of the file.

In this PR we work around the issue by not depending on std::fs::File at all. Instead, we provide tiny wrappers very similar to std::io::Stdout and std::io::Stdin around raw file descriptors (on Unixes) and file handles (on Windows). Because we work with raw system objects, it is the system that verifies their validity and we handle errors in case they are not as they should be—we do not have to worry about any safety constraints that the Rust standard library comes with.

panhania commented 6 months ago

I'm not convinced it's worth doing this to solve a problem that is ... mostly fine. But that's a call you can make as the maintainer.

Since the crate is considered UB-RISK-3 right now, which highlights with red in the Unsafe Rust Crabal reviewlog spreadsheet, it definitely causes some anxiety in me that I would like to fix :D

might be worth publishing the io stuff as a separate utility crate though

What is the benefit that you see in doing so? Since these are published to crates.io, the more crates I have, the more friction there is when it comes to making releases. Plus, I always feel a bit uneasy about yet another thing polluting the global namespace...

Manishearth commented 6 months ago

which highlights with red in the Unsafe Rust Crabal reviewlog spreadsheet, it definitely causes some anxiety in me that I would like to fix :D

I'm not convinced this change can fix that, unfortunately. The result of the discussion on this with the Rust opsem group was that this is language level UB in Rust, not library level UB that a library can opt out of.

There is some risk in using a crate that performs this operation, regardless of the libraries it uses to do so, and people importing this crate need to consider that.

(However, that risk rating really should come with some commentary explaining the situation. I'll go fix that soon)

What is the benefit that you see in doing so?

Reusable by others, mostly. And if you're not going to change it often it's not too much friction.

I wouldn't worry about the global namespace.

panhania commented 5 months ago

I'm not convinced this change can fix that, unfortunately. The result of the discussion on this with the Rust opsem group was that this is language level UB in Rust, not library level UB that a library can opt out of.

I see. In that case I think eventually I will follow up with a redesign of the library that requires explicit initialization through an unsafe function. This way the library is safe as long as the caller of the unsafe initialization can guarantee that these descriptors are what they should be. I think you touched on that in one of your previous comments.

Reusable by others, mostly. And if you're not going to change it often it's not too much friction.

Ah, I see. If it is going to be reusable, then I guess you envision it as something more generic rather than from the Fleetspeak namespace. I will file an issue for doing that as I am not sure when I will have cycles for that but sounds reasonable.