5 ms·

That's not quite right. To be safe from memory vulnerabilities, you need to understand how those 76 lines interact with the entire rest of the codebase. How har

by makomk 7y ago

That's not quite right. To be safe from memory vulnerabilities, you need to understand how those 76 lines interact with the entire rest of the codebase. How hard this is depends exactly on what interfaces they expose and to where. If I temember correctly this isn't just a theoretical issue, there was at least one memory safety issue in the Rust standard library that could only be spotted by considering the unsafe code, the safe code in that library, and how other safe code could make use of it at the same time.

derefr 7y ago
In other words, auditing one line of code within an unsafe{}-block in Rust, has about the same time-cost as auditing one line of a macro in a language that has them. You have to look at all the code that could be affected by the code, not just the code itself, because, like a macro, unsafe code “infects” other code with its semantics. (In macro code, that’s “the call sites”; in Rust’s unsafe code, that’s mostly code running concurrently to the executing code, given that you’re supposed to reintroduce a valid “safe” state before the end of the unsafe{} block.) Though, keep in mind, this isn’t something especially costly about Rust. To properly analyze any low-level language without unsafe{} requires treating every LoC to this extended audit.
- fluffything 7y ago
  Rust modules using unsafe can be pretty self-contained and small. For this particular library, most unsafe blocks are for calling glibc functions via FFI. They could probably use nix to avoid unsafe in FFI with libc.
  - cyphar 7y ago
    > They could probably use nix to avoid unsafe in FFI with libc. Though nix also just wraps the FFI unsafe blocks. I'm also very "meh" on nix -- it seemed like a neat project when I first started looking into using it but the interfaces they provide make it harder and less idiomatic to hook into libc. For instance, all of the *Set interfaces are significantly more annoying to use than just bitwise or in C.
    - fluffything 7y ago
      > Though nix also just wraps the FFI unsafe blocks. So? How they provide safe APIs doesn't matter, what matters is that they are safe. If the C FFI API is always safe for all inputs, then wrapping it in an unsafe block is the right thing to do. > but the interfaces they provide make it harder and less idiomatic to hook into libc. The interfaces they provide are both safe and portable over pretty much all UNIX-like systems (MacOSX, Linux, FreeBSD, OpenBSD, NetBSD, DragonflyBSD, Solaris, ...). libc interfaces are platform specific.
      - cyphar 7y ago
        I agree that unsafe { libc::foo() } is generally fine assuming the inputs are sane, but my point was more that nix is somewhat of a glorified unsafe wrapper that isn't always necessary. But my issue with nix wasn't portability, it's that quite a few of the APIs they provide are more-frustrating-than-necessary.
steveklabnik 7y ago
Not the entire rest of the codebase, only stuff in the same module.
- foldr 7y ago
  How does a module boundary help?
  - steveklabnik 7y ago
    Modules are the privacy boundary in Rust. This means if you’ve got some invariant you depend on, that invariant is scoped to the module. https://doc.rust-lang.org/nomicon/working-with-unsafe.html https://doc.rust-lang.org/nomicon/working-with-unsafe.html
    - foldr 7y ago
      Right, that's how a module should be written. But the purpose of an audit is to find things that aren't as they should be. If you're using a module in the standard library, or another module that's widely used, then you can reasonably assume that any unsafe behavior is encapsulated. But that's more because you know the code quality is going to be at a certain level than because of the module boundary per se.
      - steveklabnik 7y ago
        I think maybe we're splitting hairs here. The point is, the only way for something to be accessed outside of a module is through its boundary, so if something has gone wrong due to code outside, it must be to some code inside that has been exposed incorrectly. That means the fault lies inside the boundary.
        foldr 7y ago
        That is just to say that if an API that purports to be safe turns out not to be safe, then that's a problem with the API. The statement is true but tautological. Thus, I don't see how it could actually help to narrow down the source of, say, a segfault in a real-world debugging scenario. You could establish the same convention for C++. Let's say that any C++ module that exposes an unsafe API is at fault for doing so. Great, now we can localize the "blame" for any given segfault to the module containing the code that actually dereferences the invalid pointer. Of course, the bug is just as easy or hard to fix as it would have been without this semantic convention. Maybe it's an easy one line fix. Maybe modifying the module to have a safe API would require a total rewrite of the rest of the codebase.
saghm 7y ago
In general, the framework for Rust unsafe code is that you're allowed to relax certain invariants in unsafe blocks (e.g. create additional mutable pointers to something already referenced), but you're still required to restore them when the block ends. If you have two mutable references to the same thing at the end of an unsafe block, you'll won't get an issue in the unsafe code, but it's still the unsafe code that's wrong by definition. This means that all you should have to do is inspect each unsafe block and ensure that the invariants hold at the end of it. (This of course assumes no bugs in the Rust compiler with regards to safety, but if you include compiler bugs, then even 100% safe code with no unsafe blocks could potentially have an issue)
- cesarb 7y ago
  > If you have two mutable references to the same thing at the end of an unsafe block, you'll won't get an issue in the unsafe code This is wrong AFAIK: having two mutable references to the same thing is undefined behavior even within an unsafe block. An unsafe block only allows extra functionality (like dereferencing raw pointers), it doesn't change the semantics of functionality that's already allowed outside an unsafe block. That is: what would be undefined behavior outside an unsafe block is still undefined behavior inside an unsafe block.
cesarb 7y ago
> To be safe from memory vulnerabilities, you need to understand how those 76 lines interact with the entire rest of the codebase. This is explained in more detail in the Rust documentation: https://doc.rust-lang.org/nomicon/working-with-unsafe.html https://doc.rust-lang.org/nomicon/working-with-unsafe.html "[...] Because it relies on invariants of a struct field, this unsafe code does more than pollute a whole function: it pollutes a whole module. Generally, the only bullet-proof way to limit the scope of unsafe code is at the module boundary with privacy." That is, it's not enough to read just the block of code marked "unsafe". You also have to consider which invariants that block of code depends on, and all the code which could affect that invariant. But it's often easy to constrain what can affect the invariant; in the example given in that page, the invariant is that the "capacity" field in the struct must exactly match the amount of memory allocated for block of memory stored in the "pointer" field (and the "length" field must not be greater than the "capacity" field). Since neither field is "pub", only code in the same module can modify either of them without using "unsafe" itself (with "unsafe", one could in theory use "transmute" to access even private fields).

by makomk 7y ago

derefr 7y ago

In other words, auditing one line of code within an unsafe{}-block in Rust, has about the same time-cost as auditing one line of a macro in a language that has them. You have to look at all the code that could be affected by the code, not just the code itself, because, like a macro, unsafe code “infects” other code with its semantics. (In macro code, that’s “the call sites”; in Rust’s unsafe code, that’s mostly code running concurrently to the executing code, given that you’re supposed to reintroduce a valid “safe” state before the end of the unsafe{} block.) Though, keep in mind, this isn’t something especially costly about Rust. To properly analyze any low-level language without unsafe{} requires treating every LoC to this extended audit.

fluffything 7y ago
Rust modules using unsafe can be pretty self-contained and small. For this particular library, most unsafe blocks are for calling glibc functions via FFI. They could probably use nix to avoid unsafe in FFI with libc.
- cyphar 7y ago
  > They could probably use nix to avoid unsafe in FFI with libc. Though nix also just wraps the FFI unsafe blocks. I'm also very "meh" on nix -- it seemed like a neat project when I first started looking into using it but the interfaces they provide make it harder and less idiomatic to hook into libc. For instance, all of the *Set interfaces are significantly more annoying to use than just bitwise or in C.
  - fluffything 7y ago
    > Though nix also just wraps the FFI unsafe blocks. So? How they provide safe APIs doesn't matter, what matters is that they are safe. If the C FFI API is always safe for all inputs, then wrapping it in an unsafe block is the right thing to do. > but the interfaces they provide make it harder and less idiomatic to hook into libc. The interfaces they provide are both safe and portable over pretty much all UNIX-like systems (MacOSX, Linux, FreeBSD, OpenBSD, NetBSD, DragonflyBSD, Solaris, ...). libc interfaces are platform specific.
    - cyphar 7y ago
      I agree that unsafe { libc::foo() } is generally fine assuming the inputs are sane, but my point was more that nix is somewhat of a glorified unsafe wrapper that isn't always necessary. But my issue with nix wasn't portability, it's that quite a few of the APIs they provide are more-frustrating-than-necessary.

steveklabnik 7y ago

Not the entire rest of the codebase, only stuff in the same module.

foldr 7y ago
How does a module boundary help?
- steveklabnik 7y ago
  Modules are the privacy boundary in Rust. This means if you’ve got some invariant you depend on, that invariant is scoped to the module. https://doc.rust-lang.org/nomicon/working-with-unsafe.html https://doc.rust-lang.org/nomicon/working-with-unsafe.html
  - foldr 7y ago
    Right, that's how a module should be written. But the purpose of an audit is to find things that aren't as they should be. If you're using a module in the standard library, or another module that's widely used, then you can reasonably assume that any unsafe behavior is encapsulated. But that's more because you know the code quality is going to be at a certain level than because of the module boundary per se.
    - steveklabnik 7y ago
      I think maybe we're splitting hairs here. The point is, the only way for something to be accessed outside of a module is through its boundary, so if something has gone wrong due to code outside, it must be to some code inside that has been exposed incorrectly. That means the fault lies inside the boundary.
      - foldr 7y ago
        That is just to say that if an API that purports to be safe turns out not to be safe, then that's a problem with the API. The statement is true but tautological. Thus, I don't see how it could actually help to narrow down the source of, say, a segfault in a real-world debugging scenario. You could establish the same convention for C++. Let's say that any C++ module that exposes an unsafe API is at fault for doing so. Great, now we can localize the "blame" for any given segfault to the module containing the code that actually dereferences the invalid pointer. Of course, the bug is just as easy or hard to fix as it would have been without this semantic convention. Maybe it's an easy one line fix. Maybe modifying the module to have a safe API would require a total rewrite of the rest of the codebase.

saghm 7y ago

In general, the framework for Rust unsafe code is that you're allowed to relax certain invariants in unsafe blocks (e.g. create additional mutable pointers to something already referenced), but you're still required to restore them when the block ends. If you have two mutable references to the same thing at the end of an unsafe block, you'll won't get an issue in the unsafe code, but it's still the unsafe code that's wrong by definition. This means that all you should have to do is inspect each unsafe block and ensure that the invariants hold at the end of it. (This of course assumes no bugs in the Rust compiler with regards to safety, but if you include compiler bugs, then even 100% safe code with no unsafe blocks could potentially have an issue)

cesarb 7y ago
> If you have two mutable references to the same thing at the end of an unsafe block, you'll won't get an issue in the unsafe code This is wrong AFAIK: having two mutable references to the same thing is undefined behavior even within an unsafe block. An unsafe block only allows extra functionality (like dereferencing raw pointers), it doesn't change the semantics of functionality that's already allowed outside an unsafe block. That is: what would be undefined behavior outside an unsafe block is still undefined behavior inside an unsafe block.

cesarb 7y ago

> To be safe from memory vulnerabilities, you need to understand how those 76 lines interact with the entire rest of the codebase. This is explained in more detail in the Rust documentation: https://doc.rust-lang.org/nomicon/working-with-unsafe.html https://doc.rust-lang.org/nomicon/working-with-unsafe.html "[...] Because it relies on invariants of a struct field, this unsafe code does more than pollute a whole function: it pollutes a whole module. Generally, the only bullet-proof way to limit the scope of unsafe code is at the module boundary with privacy." That is, it's not enough to read just the block of code marked "unsafe". You also have to consider which invariants that block of code depends on, and all the code which could affect that invariant. But it's often easy to constrain what can affect the invariant; in the example given in that page, the invariant is that the "capacity" field in the struct must exactly match the amount of memory allocated for block of memory stored in the "pointer" field (and the "length" field must not be greater than the "capacity" field). Since neither field is "pub", only code in the same module can modify either of them without using "unsafe" itself (with "unsafe", one could in theory use "transmute" to access even private fields).