5 ms·

> The ammount of unsafe blocks is pretty small compared to the rest of the code. Which means those parts can be reviewed more extensively. If anything your argu

by DSMan195276 7y ago

> The ammount of unsafe blocks is pretty small compared to the rest of the code. Which means those parts can be reviewed more extensively. If anything your argument is pretty goofy. I've talked about this at length before, but just because you have 'less' `unsafe` code does not mean you code is any better off. Very few operations are actually `unsafe`, and you're still not allowed to break any of the (poorly or not documented) constraints that Rust requires even in `unsafe` code. Point being, unless you can make "safe" abstractions around your `unsafe` code that cannot be broken (Which is largely what you do in any language anyway, when you can...), the distinction between "safe" and "unsafe" is thin, because the parts of the code you declare `unsafe` are not the parts that are actually going to have the bugs. And because of the fact that the constraints you are required to fulfill in `unsafe` code is unclear, there's little way to guarantee your code is broken in some subtle way the optimizer might jump on (either now, or in a later release). And to be clear, I like Rust, but `unsafe` is poorly thought-out IMO.

zozbot234 7y ago
If you aren't making a safe abstraction around your unsafe code, you're supposed to mark the surrounding function "unsafe" as well, and document its expected constraints in a 'Safety' comment block. In fact, you're supposed to do the same for any piece of code that cannot provide safety guarantees about its use of 'unsafe' features. 'Plain' unsafe{ } blocks should only be used as part of building a safe abstraction.
- DSMan195276 7y ago
  > If you aren't making a safe abstraction around your unsafe code, you're supposed to mark the surrounding function "unsafe" as well, and document its expected constraints in a 'Safety' comment block. Sure, but now we're back to the issue that it's unclear what constraints `unsafe` code actually has to hold, meaning ensuring your abstraction is safe is just about impossible to do with certainty (And OS kernel code is definitely going to hit on a lot of those corner cases). You may think you have a safe interface, only to find out later you're relying on internal details that aren't guaranteed to stay the same between compiler versions or during optimizations. With that said, while I agree with what you're proposing about marking surrounding code "unsafe", it leads to lots of strange cases and most people get it wrong or will even disagree with this proposal completely. For example, it can lead to cases where you mark a function `unsafe` even though it contains nothing but "safe" code. And at that point, it's up to you to determine if something is actually "safe" or "unsafe", meaning the markings of "safe" and "unsafe" just become arbitrary choices based on what you think "unsafe" means, rather than marking something that actually does one of the "unsafe" operations. One of the best examples is pointer arithmetic. It's explicitly a safe operation, but it's also the first things most people would identify as being "unsafe", even though it is dereferencing that is the "unsafe" operation. Ex. You could easily write slice::from_raw_parts without using any `unsafe` code at all, it just puts a pointer and length together into a structure (it doesn't even need to do arithmetic!). It's only marked `unsafe` because it can break other pieces of "safe" code that it doesn't use, but it itself is 100% safe. You could just as easily argue the "other code" should be the `unsafe` code, since it's what will actually break if you use it incorrectly. Perhaps the biggest annoyance I have is that the official Rust documentation pretty much goes against the idea you presented, saying > People are fallible, and mistakes will happen, but by requiring these four unsafe operations to be inside blocks annotated with unsafe you’ll know that any errors related to memory safety must be within an unsafe block. Keep unsafe blocks small; you’ll be thankful later when you investigate memory bugs. Which is just incorrect - memory safety issues are likely to be due to the "safe" code surrounding your unsafe code - when you're writing C code, the bug isn't that you dereferenced NULL or an OOB pointer, the bug is the code that gave you that pointer in the first place, and in Rust that code is likely to all be "safe". Point being, most of the advice on keeping your unsafe blocks small just leads to people making silly APIs that can be broken via the safe API they wrap (Even in weird ways, like calling a method containing only safe code), and unfortunately there are lots of subtle ways you can unintentionally break you Rust code that most people aren't going to have any idea about.
  - zozbot234 7y ago
    > Sure, but now we're back to the issue that it's unclear what constraints `unsafe` code actually has to hold The Rust Nomicon actually documents these constraints for each 'unsafe' operation. Code that can ensure that these constraints hold can be regarded as 'safe' and rely on an unsafe{ } block. Code that can't, should be marked unsafe and document its own constraints in turn. > You may think you have a safe interface, only to find out later you're relying on internal details If you're relying on internal details, you're most likely doing it wrong, even by the standards of 'unsafe'. There are ways to nail down these details where required, at which point they're not even "internal" details anymore, but this has to be opted-into explicitly, for sensible reasons. > It's only marked `unsafe` because it can break other pieces of "safe" code that it doesn't use, but it itself is 100% safe. You could just as easily argue the "other code" should be the `unsafe` code, since it's what will actually break if you use it incorrectly. No, "other code" should not be marked unsafe because this would mean that relying on slices is inherently unsafe. Which is silly; the whole point of a "slice" type, contrasted with its "raw parts", is in the guarantees it provides. This is why slice::from_raw_parts is unsafe: not because if what it does, but what it states about the result. > Which is just incorrect - memory safety issues are likely to be due to the "safe" code surrounding your unsafe code This is a matter of how you assign "blame" for a memory safety error. It's definitely true however, that the memory unsafe operations play a key role, and I assume that's the point that these docs are making. I agree that people shouldn't be creating faulty abstractions around "unsafe" blocks, but the reason we can even identify this as an issue is because we have "unsafe" blocks in the first place!
    - DSMan195276 7y ago
      > The Rust Nomicon actually documents these constraints for each 'unsafe' operation. Code that can ensure that these constraints hold can be regarded as 'safe' and rely on an unsafe{ } block. Code that can't, should be marked unsafe and document its own constraints in turn. > > If you're relying on internal details, you're most likely doing it wrong, even by the standards of 'unsafe'. There are ways to nail down these details where required, at which point they're not even "internal" details anymore, but this has to be opted-into explicitly, for sensible reasons. It's not that simple, what I'm getting at is that 'unsafe' code is still required to meet all the constraints that 'safe' code has to meet, even though those constraints are not fully documented (Because in general, in 'safe' code there are lots of things that are impossible to do but not explicitly UB), and they have changed them in significant ways in the past. You can see an incomplete list of them here[0]. There has been work to nail these details down for years, with the most recent being this one[1], but in general I would argue there hasn't been much progress since I first looked into it years ago now. Fun fact, the first Rust issue I remember reading that touched on this (and evolved into basically everything going on today) is this[4] one, which was made exactly 4 years ago today (And I mean exact!). The 'Safety' sections for 'unsafe' operations are actually just extra requirements for those particular API on top that you also have to keep in addition to whatever safe Rust constraints apply. And I would argue in general they're not actually exhaustive, and they're mostly just a "best guess" - it's not a breaking change to introduce or realize there are more constraints. Ex. A little more then a year ago, they added the info that a slice can't be larger than `isize::MAX`[2], and a few months ago was this[3] one, where they added that the pointer length must be from a single allocation. My point is that when you're doing something like writing an OS kernel, you end up running into these types of issues and corner-cases because Ex. They determine what your allocator is allowed to produce, or if some particular pointer can be validly put into a slice. > No, "other code" should not be marked unsafe because this would mean that relying on slices is inherently unsafe. Which is silly; the whole point of a "slice" type, contrasted with its "raw parts", is in the guarantees it provides. This is why slice::from_raw_parts is unsafe: not because if what it does, but what it states about the result. Sure, my point is that this is at best unclear, and I would argue wildly misunderstood. We've basically made two definitions of 'unsafe' - one for code that has to perform one of the five 'unsafe' operations (Which is a clearly defined 'yes' or 'no', and also the one described by the Rust docs), and one for code that can potentially break some type of invariant the will potentially cause UB somewhere else in the program (even if the code in question is completely safe and can't on its own cause any problems). The problem is that Rust only guarantees the type of safety that it does if you do the second, but yet lots of people (I would almost argue most...) assume the first is sufficient and 'unsafe' by itself ensures all your memory bugs are in `unsafe` code. The Rust docs even describes safe vs. unsafe as two completely different languages (which they are) which brings into question why you would write `unsafe` code when you don't even need any of the `unsafe` operations. You arguably want a separate keyword for this - one that marks a function `unsafe`, but doesn't actually allow you to perform any of the `unsafe` operations in it. [0] https://doc.rust-lang.org/nomicon/what-unsafe-does.html https://doc.rust-lang.org/nomicon/what-unsafe-does.html [1] https://github.com/rust-lang/unsafe-code-guidelines https://github.com/rust-lang/unsafe-code-guidelines [2] https://github.com/rust-lang/rust/commit/1975b8d21b21d5ede54494d0b7661d6ac14ec6a2 https://github.com/rust-lang/rust/commit/1975b8d21b21d5ede54... [3] https://github.com/rust-lang/rust/commit/1a254e4f434e0cbb9ffcb24971416cb574df4751 https://github.com/rust-lang/rust/commit/1a254e4f434e0cbb9ff... [4] https://github.com/rust-lang/rfcs/issues/1447 https://github.com/rust-lang/rfcs/issues/1447
swsieber 7y ago
It doesn't mean your code is better off, but it makes auditing your code a lot easier. Rust gives you lots of good tools so that you can fence in your unsafe and restrict it to a module (source code file), such that if you audit the module containing the unsafe, you should be good to go.

zozbot234 7y ago

If you aren't making a safe abstraction around your unsafe code, you're supposed to mark the surrounding function "unsafe" as well, and document its expected constraints in a 'Safety' comment block. In fact, you're supposed to do the same for any piece of code that cannot provide safety guarantees about its use of 'unsafe' features. 'Plain' unsafe{ } blocks should only be used as part of building a safe abstraction.

DSMan195276 7y ago
> If you aren't making a safe abstraction around your unsafe code, you're supposed to mark the surrounding function "unsafe" as well, and document its expected constraints in a 'Safety' comment block. Sure, but now we're back to the issue that it's unclear what constraints `unsafe` code actually has to hold, meaning ensuring your abstraction is safe is just about impossible to do with certainty (And OS kernel code is definitely going to hit on a lot of those corner cases). You may think you have a safe interface, only to find out later you're relying on internal details that aren't guaranteed to stay the same between compiler versions or during optimizations. With that said, while I agree with what you're proposing about marking surrounding code "unsafe", it leads to lots of strange cases and most people get it wrong or will even disagree with this proposal completely. For example, it can lead to cases where you mark a function `unsafe` even though it contains nothing but "safe" code. And at that point, it's up to you to determine if something is actually "safe" or "unsafe", meaning the markings of "safe" and "unsafe" just become arbitrary choices based on what you think "unsafe" means, rather than marking something that actually does one of the "unsafe" operations. One of the best examples is pointer arithmetic. It's explicitly a safe operation, but it's also the first things most people would identify as being "unsafe", even though it is dereferencing that is the "unsafe" operation. Ex. You could easily write slice::from_raw_parts without using any `unsafe` code at all, it just puts a pointer and length together into a structure (it doesn't even need to do arithmetic!). It's only marked `unsafe` because it can break other pieces of "safe" code that it doesn't use, but it itself is 100% safe. You could just as easily argue the "other code" should be the `unsafe` code, since it's what will actually break if you use it incorrectly. Perhaps the biggest annoyance I have is that the official Rust documentation pretty much goes against the idea you presented, saying > People are fallible, and mistakes will happen, but by requiring these four unsafe operations to be inside blocks annotated with unsafe you’ll know that any errors related to memory safety must be within an unsafe block. Keep unsafe blocks small; you’ll be thankful later when you investigate memory bugs. Which is just incorrect - memory safety issues are likely to be due to the "safe" code surrounding your unsafe code - when you're writing C code, the bug isn't that you dereferenced NULL or an OOB pointer, the bug is the code that gave you that pointer in the first place, and in Rust that code is likely to all be "safe". Point being, most of the advice on keeping your unsafe blocks small just leads to people making silly APIs that can be broken via the safe API they wrap (Even in weird ways, like calling a method containing only safe code), and unfortunately there are lots of subtle ways you can unintentionally break you Rust code that most people aren't going to have any idea about.
- zozbot234 7y ago
  > Sure, but now we're back to the issue that it's unclear what constraints `unsafe` code actually has to hold The Rust Nomicon actually documents these constraints for each 'unsafe' operation. Code that can ensure that these constraints hold can be regarded as 'safe' and rely on an unsafe{ } block. Code that can't, should be marked unsafe and document its own constraints in turn. > You may think you have a safe interface, only to find out later you're relying on internal details If you're relying on internal details, you're most likely doing it wrong, even by the standards of 'unsafe'. There are ways to nail down these details where required, at which point they're not even "internal" details anymore, but this has to be opted-into explicitly, for sensible reasons. > It's only marked `unsafe` because it can break other pieces of "safe" code that it doesn't use, but it itself is 100% safe. You could just as easily argue the "other code" should be the `unsafe` code, since it's what will actually break if you use it incorrectly. No, "other code" should not be marked unsafe because this would mean that relying on slices is inherently unsafe. Which is silly; the whole point of a "slice" type, contrasted with its "raw parts", is in the guarantees it provides. This is why slice::from_raw_parts is unsafe: not because if what it does, but what it states about the result. > Which is just incorrect - memory safety issues are likely to be due to the "safe" code surrounding your unsafe code This is a matter of how you assign "blame" for a memory safety error. It's definitely true however, that the memory unsafe operations play a key role, and I assume that's the point that these docs are making. I agree that people shouldn't be creating faulty abstractions around "unsafe" blocks, but the reason we can even identify this as an issue is because we have "unsafe" blocks in the first place!
  - DSMan195276 7y ago
    > The Rust Nomicon actually documents these constraints for each 'unsafe' operation. Code that can ensure that these constraints hold can be regarded as 'safe' and rely on an unsafe{ } block. Code that can't, should be marked unsafe and document its own constraints in turn. > > If you're relying on internal details, you're most likely doing it wrong, even by the standards of 'unsafe'. There are ways to nail down these details where required, at which point they're not even "internal" details anymore, but this has to be opted-into explicitly, for sensible reasons. It's not that simple, what I'm getting at is that 'unsafe' code is still required to meet all the constraints that 'safe' code has to meet, even though those constraints are not fully documented (Because in general, in 'safe' code there are lots of things that are impossible to do but not explicitly UB), and they have changed them in significant ways in the past. You can see an incomplete list of them here[0]. There has been work to nail these details down for years, with the most recent being this one[1], but in general I would argue there hasn't been much progress since I first looked into it years ago now. Fun fact, the first Rust issue I remember reading that touched on this (and evolved into basically everything going on today) is this[4] one, which was made exactly 4 years ago today (And I mean exact!). The 'Safety' sections for 'unsafe' operations are actually just extra requirements for those particular API on top that you also have to keep in addition to whatever safe Rust constraints apply. And I would argue in general they're not actually exhaustive, and they're mostly just a "best guess" - it's not a breaking change to introduce or realize there are more constraints. Ex. A little more then a year ago, they added the info that a slice can't be larger than `isize::MAX`[2], and a few months ago was this[3] one, where they added that the pointer length must be from a single allocation. My point is that when you're doing something like writing an OS kernel, you end up running into these types of issues and corner-cases because Ex. They determine what your allocator is allowed to produce, or if some particular pointer can be validly put into a slice. > No, "other code" should not be marked unsafe because this would mean that relying on slices is inherently unsafe. Which is silly; the whole point of a "slice" type, contrasted with its "raw parts", is in the guarantees it provides. This is why slice::from_raw_parts is unsafe: not because if what it does, but what it states about the result. Sure, my point is that this is at best unclear, and I would argue wildly misunderstood. We've basically made two definitions of 'unsafe' - one for code that has to perform one of the five 'unsafe' operations (Which is a clearly defined 'yes' or 'no', and also the one described by the Rust docs), and one for code that can potentially break some type of invariant the will potentially cause UB somewhere else in the program (even if the code in question is completely safe and can't on its own cause any problems). The problem is that Rust only guarantees the type of safety that it does if you do the second, but yet lots of people (I would almost argue most...) assume the first is sufficient and 'unsafe' by itself ensures all your memory bugs are in `unsafe` code. The Rust docs even describes safe vs. unsafe as two completely different languages (which they are) which brings into question why you would write `unsafe` code when you don't even need any of the `unsafe` operations. You arguably want a separate keyword for this - one that marks a function `unsafe`, but doesn't actually allow you to perform any of the `unsafe` operations in it. [0] https://doc.rust-lang.org/nomicon/what-unsafe-does.html https://doc.rust-lang.org/nomicon/what-unsafe-does.html [1] https://github.com/rust-lang/unsafe-code-guidelines https://github.com/rust-lang/unsafe-code-guidelines [2] https://github.com/rust-lang/rust/commit/1975b8d21b21d5ede54494d0b7661d6ac14ec6a2 https://github.com/rust-lang/rust/commit/1975b8d21b21d5ede54... [3] https://github.com/rust-lang/rust/commit/1a254e4f434e0cbb9ffcb24971416cb574df4751 https://github.com/rust-lang/rust/commit/1a254e4f434e0cbb9ff... [4] https://github.com/rust-lang/rfcs/issues/1447 https://github.com/rust-lang/rfcs/issues/1447

swsieber 7y ago

It doesn't mean your code is better off, but it makes auditing your code a lot easier. Rust gives you lots of good tools so that you can fence in your unsafe and restrict it to a module (source code file), such that if you audit the module containing the unsafe, you should be good to go.