16 ms·

Comparative unsafety

52 points by lionkor 6y ago

einpoklum 6y ago
> I wrote some rust code. Actually, he wrote some Rust and some C code wrapped in Rust. > I was writing an smtp server... the kind of smtp server you'd write in two days. (sigh) > The error would have been detected far sooner had I not been lazy and checked the return value of chown (for a file not found error). But [excused here] You can't, and mustn't avoid C error checking when writing C code, even if it's wrapped in Rust. I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not optional. It's very tempting to assume your standard library function just works, always.
- thaumasiotes 6y ago
  >> The error would have been detected far sooner had I not been lazy and checked the return value of chown (for a file not found error). But [excused here] > You can't, and mustn't [...] It doesn't really sound like he's trying to excuse anything. Quote the intro more fully: >>> I used unsafe. It was unsafe. After months of contemplating this unfortunate result, I've found someone else to blame.
- im3w1l 6y ago
  > I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not optional. It's very tempting to assume your standard library function just works, always. The situation is worse than that. People check for errors except when they don't. And you are expected to know the difference. Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done. Or take close(2). > If close() is interrupted by a signal that is to be caught, it shall return -1 with errno set to EINTR and the state of fildes is unspecified. > This permits the behavior that occurs on Linux and many other implementations, where, as with other errors that may be reported by close(), the file descriptor is guaranteed to be closed. However, it also permits another possibility: that the implementation returns an EINTR error and keeps the file descriptor open. (According to its documentation, HP-UX's close() does this.) The caller must then once more use close() to close the file descriptor, to avoid file descriptor leaks. This divergence in implementation behaviors provides a difficult hurdle for portable applications, since on many implementations, close() must not be called again after an EINTR error, and on at least one, close() must be called again. How did this discrepancy go unnoticed for so long? Because no one checks close return value. Everyone nods and say you should, and then keep writing code without checks, because come one close never fails. If you do check you are the weird edge case. The one people whisper about "why does he have to be such an annoying pedant". And while I'm ranting, the same goes for undefined behavior. Of course you shouldn't rely on undefined behavior. Of course of course. But maybe, just a little pointer casting is fine?
  - ncmncm 6y ago
    No one checks close()'s result because it lies. What should you write, while (close(fd) != 0) {} ? Or does it risk getting an infinite loop if, e.g., some network connection disappears? Maybe if (fsync(fd) == 0) while (close(fd) != 0) {} else abort(); But maybe fsync is interruptible too. Ultimately there is nothing strictly correct to write, and you should just not really count on files being closed before process termination, but call close() as an optimization not to leak too many fds.
    - cesarb 6y ago
      You cannot retry close(). Quoting from https://man7.org/linux/man-pages/man2/close.2.html#NOTES https://man7.org/linux/man-pages/man2/close.2.html#NOTES Retrying the close() after a failure return is the wrong thing to do, since this may cause a reused file descriptor from another thread to be closed. This can occur because the Linux kernel always releases the file descriptor early in the close operation, freeing it for reuse; the steps that may return an error, such as flushing data to the filesystem or device, occur only later in the close operation. That is, once you call close(), the file descriptor is always freed (even if close() returns an error), and can be reused for an open() or similar in another thread. Retrying the close() will fail with EBADF in the best case, and close something another thread has opened in the worst case.
      - OskarS 6y ago
        So what is the point of it having a return value at all then?
    - rectang 6y ago
      > No one checks close()'s result because it lies. And since nobody checks the result of close and very few check the result of every single write operation, many "disk full" errors go unnoticed.
      - ncmncm 6y ago
marvel_boy 6y ago
>Teaching mode requires the stove be preheated to the optimal temperature. Very poetic. Are you Russian?
thaumasiotes 6y ago
> Why am I using my own ffi version of chown instead of the libc crate? The libc crate prototypes chown with unsigned uid_t and gid_t types, and I want to pass -1 because I'm not interested in changing the group. I'll spare you the long rant about how one should never redeclare system interfaces if you can't take the time to do so properly, because it turns out if you dig into it, uid_t boils down to uint32_t, but even so, the manual for chown says -1 should work and I want it to work. First thing I'd try would be to pass "~0" instead of "-1". Would that work? (Looks like in Rust that would be "!0". I don't know anything about Rust.)
- adwn 6y ago
  Yes, !0 would work. Or, to make the intent even more clear: -1_i32 as u32 Definitely better than declaring your own function prototype.
- ChrisSD 6y ago
  In Rust the simplest way would be to use the `u32::MAX` constant. Create a fun alias for it if you want to express intent.
  - thaumasiotes 6y ago
    I actually find ~0 more intuitive. u32::MAX makes me think the relevant concept is "a really big number"; ~0 makes me think the relevant concept is "a bunch of bits that are all 1s".
    - ChrisSD 6y ago
      Fair enough. But tbh I'm not sure it really matters in this context. It's just a constant that means "ignore this parameter". Whether it's "a really big number" or "a bunch of bits that are all 1s" is incidental. Btw, you can directly express "all ones" in Rust as: 0b_1111_1111_1111_1111_1111_1111_1111_1111 Though it's somewhat less succinct. ;) EDIT: Added more ones, thanks adwn!
      - 6y ago
AndrewDucker 6y ago
This has the most toxic UI I've seen in my life. Switching away from my browser and back again causes the page to re-render, losing my place as it did so. Just awful!
- nicoburns 6y ago
  I'm pretty sure it's an artificial loading screen too. There's no way it actually takes that long to render. It's quite an interesting artistic statement.
  - tyingq 6y ago
    Yep. window.setTimeout(makeprogress, delay) With various repeated hard-coded delays of 50, 250, and 500ms.
- tyingq 6y ago
  Extra maddening, as it works fine with Javascript disabled. Those loading and rendering progress bars are a terrible idea.
  - stefan_ 6y ago
    Haha, it's true! Disabling JavaScript leaves a perfectly working article. Is this one big troll?
  - caslon 6y ago
    It's a joke. It's not a terrible idea. It's a great idea! A great joke!
    - tyingq 6y ago
      Well, aside from the delay in seeing the content, it borks the back button up, etc. Breaking all the pages on your site for a joke is an odd idea to me. Edit: As mentioned by another commenter, seems to be deliberate punishment for having javascript enabled. Also, clicking outside the main window (like in the url bar) does the render thing again. And a search for javascript has some odd results too: https://flak.tedunangst.com/search?q=javascript https://flak.tedunangst.com/search?q=javascript Last, there's some deliberately awful infinite scrolling too.
wyldfire 6y ago
I will concede that it would be nice if I could opt-in to a 'be-really-nitpicky' mode for clippy. I would even do it sometimes. Usually I just want something with a little higher SNR.
- pornel 6y ago
  Good news! cargo clippy -- -W clippy::pedantic or `#![warn(clippy::pedantic)]` in your crate. Here's a list of lints and groups you can turn on/off: https://rust-lang.github.io/rust-clippy/master/index.html https://rust-lang.github.io/rust-clippy/master/index.html
nicoburns 6y ago
From my perspective as someone who knows Rust but primarily works with high-level languages like JavaScript, some developers who from a background of unsafe languages (particularly C and C++) seem incredibly cavalier around `unsafe` blocks in Rust code. It's like they're desensitised to the unsafety. As someone who is used to working in a language where one absolutely cannot hit memory safety issues or undefined behaviour, you can be damn sure that I'm going to read all the relevant documentation and double/triple the invariants of any unsafe code I write in Rust (and maybe even get it reviewed by someone else). In some ways unsafe blocks in Rust are more unsafe than the equivalent C or C++ code because you have to uphold Rust's more stringent safety guarantees (e.g. no aliasing of mutable references), but they come with the saving grace that you don't need them very often, so you can afford to really take your time and implement them thoroughly.
- dundarious 6y ago
  Maybe that's all true, but in the specific example in question, he just does a string conversion and calls into a C library via ffi (for a reason he has justified). I think he has a real point about how the compiler should find this error if Rust is to fully match its marketing, so to speak. Clippy (a tool for style issues as much as anything else) doesn't seem to be an appropriate place for such error checking. On balance Rust does a great job, but "be even more careful in unsafe Rust than in regular C++" is probably a losing battle.
- cesarb 6y ago
  > From my perspective as someone who knows Rust but primarily works with high-level languages like JavaScript, some developers who from a background of unsafe languages (particularly C and C++) seem incredibly cavalier around `unsafe` blocks in Rust code. It's like they're desensitised to the unsafety. That's true. If you are used to 100% of your code being within the equivalent of a Rust "unsafe" block, having over 90% of your code being verified as safe by the compiler is a luxury. Even if one in ten lines are marked as "unsafe", that's already many times less "unsafe" than what they are used to in their C or C++ code. Moreover, C and C++ developers are used to all kinds of bizarre memory-saving and cycle-counting code that require "unsafe" in Rust. Things like intrusive double-linked circular linked lists, stashing things in the lower bits of pointers (I mean, it's a pointer to a 32-bit value which will always be 32-bit aligned, the lowest two bits will always be zero, why not use them?), doing XOR on pointers, and plenty of other useful tricks.
joosters 6y ago
Rendering a web page requires an in-page animated progress bar now?
- skrebbel 6y ago
  Only if you have JavaScript enabled.
- horsawlarway 6y ago
  And restarts the whole process every time you change focus away from the window (at least from chrome on win10)
  - frosted-flakes 6y ago
    And also loses your scroll position.
- camgunz 6y ago
  It's just whiz-bang, doesn't actually do anything. I kinda like it!
howeyc 6y ago
Wait, what? I don't know rust, at all, but why is path freed? I thought rust didn't have GC? > let path = CString::new(filename.as_str()).unwrap().as_ptr(); Is one of those functions calling free behind the scenes?
- deleted 6y ago
  [deleted]
- OskarS 6y ago
  I mean, yeah, at the end you only get a raw pointer, the object managing the pointer doesn't exist anymore. I'm not a Rust programmer, but I would imagine it's equivalent to the following C++ code auto ptr = std::string(something).c_str(); This creates a std::string, calls c_str() on it to get the pointer. However, since the std::string isn't stored anywhere, its lifetime stops and it is destructed (this is why these are called temporaries), and the pointer is now invalid. The right way of doing it is this: std::string str { something }; auto ptr = str.c_str(); Now, str and ptr both live in the same scope, and ptr's lifetime is entirely contained in str's.
- remexre 6y ago
  like C++, rust frees values once they've gone out of scope; the result of CString::new(filename.as_str()).unwrap() is a temporary which is freed at the end of that line, since the .as_ptr() returns a non-lifetime-encumbered value.
- cesarb 6y ago
  It's freed for the same reason it would be freed in C++. Like C++, Rust relies heavily on RAII, in which resources are freed once they get out of scope. The "CString::new(filename.as_str()).unwrap()" returns a CString, which owns the memory for the C string. Since it's a temporary (it's not being stored anywhere), its scope ends before the next line, so the resources it owns are freed (by calling its Drop implementation) just after the ".as_ptr()" call. One solution would be to split it into two lines: let path = CString::new(filename.as_str()).unwrap(); let path = path.as_ptr(); That way, the CString would only release its resources at the end of the block. With references, the borrow checker doesn't let you do it the wrong way; however, the borrow checker only applies to references, not to raw pointers (which is what .as_ptr() returns).
brundolf 6y ago
> I used it for a while, but generally found it too tiresome. Worth noting that you can disable individual Clippy warnings instead of the whole thing (per line or for the whole project). I almost always turn off the dead_code warning, especially when I'm iterating
rrss 6y ago
> If there's an error which is a clear cut bug, I think it should be reported by an error detecting tool, not a linter Relevant rustc (merged) PR: https://github.com/rust-lang/rust/pull/75671 https://github.com/rust-lang/rust/pull/75671 - "Uplift temporary-cstring-as-ptr lint from clippy into rustc"
- rectang 6y ago
  This seems like a positive development, but there are other `as_ptr` and `as_mut_ptr` functions. The one that I tripped up with was actually from Vec, not CString. Zooming out, there are innumerable ways to create a dangling pointer. This is really a vexing problem.
  - Jweb_Guru 6y ago
    Rust doesn't intend to prevent you from creating dangling pointers... if you want that functionality, use references. The reason this issue is particularly likely to hit is that it's the intersection of three things: (1) one of the very few times when people often need raw pointers when not writing very carefully inspected unsafe code is when calling functions across an FFI boundary, (2) C strings are represented by a char * pointer using a type (3) a Rust value that's created as a temporary will drop on the same line. (1) and (2) are how people can know it's almost always a bug when someone does this with a `CString`, whereas a lint would probably have a lot more false positives for something like a `Vec` (which is rarely passed to C directly since it doesn't understand it). Keep in mind that even something like borrowing a RefCell creates a temporary, so once you cast to a pointer and end the lifetime it came from it's very hard for the type system to track back the pointer you got to any particular deallocated temporary in an intelligent way. It pretty much has to be done on a case by case basis, I suspect (but maybe that could be improved--it is definitely the case, from a study someone did recently, that a large percentage of UB in unsafe Rust is due to destructors running early unexpectedly!).
rectang 6y ago
In Rust, dereferencing a pointer is unsafe and so must be isolated within an `unsafe` block — but creating a dangling pointer is not unsafe. What this means is that as soon as you add `unsafe` code which accepts a pointer argument, all the supposedly "safe" code which can affect this pointer argument becomes a potential source of memory errors. To me, this was unintuitive — I expected the `unsafe` block to require an extra level of scrutiny, not the surrounding code. After making an error similar to the one described in this post, I wondered, "why isn't creating a dangling pointer unsafe?" But after following that train of thought I realized that vast amounts of code would be pulled into `unsafe` blocks as a consequence, so it's not a viable approach — and thus it became apparent why only dereferencing is unsafe. The lesson seems to be that dereferencing requires extreme care, but I wish potential errors weren't so subtle. The `as_ptr` family of functions has been tripping people up for a long time: https://users.rust-lang.org/t/cstring-as-ptr-is-incredibly-unsafe/11431 https://users.rust-lang.org/t/cstring-as-ptr-is-incredibly-u...
- stefan_ 6y ago
  In general unsafe should be viral: you can't call unsafe without yourself being unsafe. But it hits the same "snag", then nothing works anymore.
  - nynx 6y ago
    This is not true. Rust is based around the idea of encapsulating unsafety within interfaces that are safe to use.
  - Jweb_Guru 6y ago
    If `unsafe` were viral it would be useless. Syntactically ill-typed code can still be proven semantically well-typed, and `unsafe` is intended as a marker that the proof of semantic soundness lies outside of Rust's type system. If there is no proof (at least an informal one), you shouldn't be using unsafe, generally speaking.
- jdc 6y ago
  I wonder what a good approach to safe pointer arithmetic would be.
mqus 6y ago
I'm somewhat wishing for a `.bind!()` macro or something similar, which will desugar `let y = x.method().bind!().reference_and_do_something()` into `let tmp = x.method(); let y = tmp.reference_and_do_something()` . This should be available for chaining for convenience. But I see the issues (e.g. which scope should `tmp` be bound to/what is its lifetime?)
nyanpasu64 6y ago
Fun fact: I hit the exact same bug when working on FamiTracker in C++, with a MFC type also called CString... "construct a CString, acquire a pointer into the internals, destroy the CString". The only reason it doesn't make FamiTracker crash is because MFC's CString is COW/refcounted, and the refcount never reached zero because it was constructed from a long-lasting string literal, so the underlying buffer was still valid after destroying the string object. I feel that tying deallocations to destructors is a benefit to safe code, and a hindrance to unsafe code. I feel explicit calls to "destructor" functions that move self (perhaps using defer), plus linear typing to statically ensure you don't forget to call a destructor or call one twice, can provide safety in both situations, but at the expense of verbosity and not interacting well with exceptions. I still believe linear typing (argued against by Gankra in https://gankra.github.io/blah/linear-rust/ https://gankra.github.io/blah/linear-rust/) will fix multiple deficiencies in Rust: - It helps write allocation-free code which passes objects around without creating/destroying them (maybe an Alloc effect would help too). - Making drops explicit makes it harder to screw up when writing unsafe code performing allocations/deallocations. It may or may not help protect against double-frees that occur upon panicking. - File's Drop implementation ignores the return code of fclose(), since you can't return values from a destructor. So dropping a File directly should be prohibited (except during panic-unwinding, which I feel is a rare scenario where perfect error-checking is less crucial than not risking more panics). Instead you move the file into a close() method/function, and are expected to check the Result<(), error> that comes out. - From what I've read, trying to perform an asynchronous cancellation operation in the standard Drop method is not possible, since the method lacks access to the executor (unless you hard-code an executor to block on). It would make sense to make explicit dropping illegal (as a lint) and require moving it into a "drop"-like function, but keep Drop around to be called during panic-unwinding. This may result in logically incorrect behavior, but is still memory-safe. And I think it's better than making your async-fn state machines always larger to fit awaiting-a-drop states, and awaiting the async runtime when you panic-unwind.
- rectang 6y ago
  > File's Drop implementation ignores the return code of fclose() Yeah, and it bothers me that I can't call `close` and check for errors. All you need is something like this: pub fn close(self) -> io::Result<()> { let status = unsafe { libc::close(self.fd) }; mem::forget(self); // prevent drop and invalid double-close match status { 0 => Ok(()), _ => Err(io::Error::new(ErrorKind::Other)), } } Note that this function takes `self` not `&mut self`, so it consumes the resource and prevents further use. Then `mem::forget` prevents `drop` from running. See discussion at https://github.com/rust-lang/rust/issues/59567 https://github.com/rust-lang/rust/issues/59567
swader999 6y ago
This guy writes well and codes dangerously. Would have a beer with.
cryptonector 6y ago
What a great blog post. This happens in garbage collected languages too. I've been bitten by this in Java (JNI). Yes, this needs to be caught by the compiler, damn it. And linters shouldn't complain about style either. Style checking should be a separate and very configurable tool.

52 points by lionkor 6y ago

> I wrote some rust code. Actually, he wrote some Rust and some C code wrapped in Rust. > I was writing an smtp server... the kind of smtp server you'd write in two days. (sigh) > The error would have been detected far sooner had I not been lazy and checked the return value of chown (for a file not found error). But [excused here] You can't, and mustn't avoid C error checking when writing C code, even if it's wrapped in Rust. I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not optional. It's very tempting to assume your standard library function just works, always.

thaumasiotes 6y ago
>> The error would have been detected far sooner had I not been lazy and checked the return value of chown (for a file not found error). But [excused here] > You can't, and mustn't [...] It doesn't really sound like he's trying to excuse anything. Quote the intro more fully: >>> I used unsafe. It was unsafe. After months of contemplating this unfortunate result, I've found someone else to blame.
im3w1l 6y ago
> I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not optional. It's very tempting to assume your standard library function just works, always. The situation is worse than that. People check for errors except when they don't. And you are expected to know the difference. Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done. Or take close(2). > If close() is interrupted by a signal that is to be caught, it shall return -1 with errno set to EINTR and the state of fildes is unspecified. > This permits the behavior that occurs on Linux and many other implementations, where, as with other errors that may be reported by close(), the file descriptor is guaranteed to be closed. However, it also permits another possibility: that the implementation returns an EINTR error and keeps the file descriptor open. (According to its documentation, HP-UX's close() does this.) The caller must then once more use close() to close the file descriptor, to avoid file descriptor leaks. This divergence in implementation behaviors provides a difficult hurdle for portable applications, since on many implementations, close() must not be called again after an EINTR error, and on at least one, close() must be called again. How did this discrepancy go unnoticed for so long? Because no one checks close return value. Everyone nods and say you should, and then keep writing code without checks, because come one close never fails. If you do check you are the weird edge case. The one people whisper about "why does he have to be such an annoying pedant". And while I'm ranting, the same goes for undefined behavior. Of course you shouldn't rely on undefined behavior. Of course of course. But maybe, just a little pointer casting is fine?
- ncmncm 6y ago
  No one checks close()'s result because it lies. What should you write, while (close(fd) != 0) {} ? Or does it risk getting an infinite loop if, e.g., some network connection disappears? Maybe if (fsync(fd) == 0) while (close(fd) != 0) {} else abort(); But maybe fsync is interruptible too. Ultimately there is nothing strictly correct to write, and you should just not really count on files being closed before process termination, but call close() as an optimization not to leak too many fds.
  - cesarb 6y ago
    You cannot retry close(). Quoting from https://man7.org/linux/man-pages/man2/close.2.html#NOTES https://man7.org/linux/man-pages/man2/close.2.html#NOTES Retrying the close() after a failure return is the wrong thing to do, since this may cause a reused file descriptor from another thread to be closed. This can occur because the Linux kernel always releases the file descriptor early in the close operation, freeing it for reuse; the steps that may return an error, such as flushing data to the filesystem or device, occur only later in the close operation. That is, once you call close(), the file descriptor is always freed (even if close() returns an error), and can be reused for an open() or similar in another thread. Retrying the close() will fail with EBADF in the best case, and close something another thread has opened in the worst case.
    - OskarS 6y ago
      So what is the point of it having a return value at all then?
  - rectang 6y ago
    > No one checks close()'s result because it lies. And since nobody checks the result of close and very few check the result of every single write operation, many "disk full" errors go unnoticed.
    - ncmncm 6y ago

marvel_boy 6y ago

>Teaching mode requires the stove be preheated to the optimal temperature. Very poetic. Are you Russian?

thaumasiotes 6y ago

> Why am I using my own ffi version of chown instead of the libc crate? The libc crate prototypes chown with unsigned uid_t and gid_t types, and I want to pass -1 because I'm not interested in changing the group. I'll spare you the long rant about how one should never redeclare system interfaces if you can't take the time to do so properly, because it turns out if you dig into it, uid_t boils down to uint32_t, but even so, the manual for chown says -1 should work and I want it to work. First thing I'd try would be to pass "~0" instead of "-1". Would that work? (Looks like in Rust that would be "!0". I don't know anything about Rust.)

adwn 6y ago
Yes, !0 would work. Or, to make the intent even more clear: -1_i32 as u32 Definitely better than declaring your own function prototype.
ChrisSD 6y ago
In Rust the simplest way would be to use the `u32::MAX` constant. Create a fun alias for it if you want to express intent.
- thaumasiotes 6y ago
  I actually find ~0 more intuitive. u32::MAX makes me think the relevant concept is "a really big number"; ~0 makes me think the relevant concept is "a bunch of bits that are all 1s".
  - ChrisSD 6y ago
    Fair enough. But tbh I'm not sure it really matters in this context. It's just a constant that means "ignore this parameter". Whether it's "a really big number" or "a bunch of bits that are all 1s" is incidental. Btw, you can directly express "all ones" in Rust as: 0b_1111_1111_1111_1111_1111_1111_1111_1111 Though it's somewhat less succinct. ;) EDIT: Added more ones, thanks adwn!
    - 6y ago

AndrewDucker 6y ago

This has the most toxic UI I've seen in my life. Switching away from my browser and back again causes the page to re-render, losing my place as it did so. Just awful!

nicoburns 6y ago
I'm pretty sure it's an artificial loading screen too. There's no way it actually takes that long to render. It's quite an interesting artistic statement.
- tyingq 6y ago
  Yep. window.setTimeout(makeprogress, delay) With various repeated hard-coded delays of 50, 250, and 500ms.
tyingq 6y ago
Extra maddening, as it works fine with Javascript disabled. Those loading and rendering progress bars are a terrible idea.
- stefan_ 6y ago
  Haha, it's true! Disabling JavaScript leaves a perfectly working article. Is this one big troll?
- caslon 6y ago
  It's a joke. It's not a terrible idea. It's a great idea! A great joke!
  - tyingq 6y ago
    Well, aside from the delay in seeing the content, it borks the back button up, etc. Breaking all the pages on your site for a joke is an odd idea to me. Edit: As mentioned by another commenter, seems to be deliberate punishment for having javascript enabled. Also, clicking outside the main window (like in the url bar) does the render thing again. And a search for javascript has some odd results too: https://flak.tedunangst.com/search?q=javascript https://flak.tedunangst.com/search?q=javascript Last, there's some deliberately awful infinite scrolling too.

wyldfire 6y ago

I will concede that it would be nice if I could opt-in to a 'be-really-nitpicky' mode for clippy. I would even do it sometimes. Usually I just want something with a little higher SNR.

pornel 6y ago
Good news! cargo clippy -- -W clippy::pedantic or `#![warn(clippy::pedantic)]` in your crate. Here's a list of lints and groups you can turn on/off: https://rust-lang.github.io/rust-clippy/master/index.html https://rust-lang.github.io/rust-clippy/master/index.html

nicoburns 6y ago

From my perspective as someone who knows Rust but primarily works with high-level languages like JavaScript, some developers who from a background of unsafe languages (particularly C and C++) seem incredibly cavalier around `unsafe` blocks in Rust code. It's like they're desensitised to the unsafety. As someone who is used to working in a language where one absolutely cannot hit memory safety issues or undefined behaviour, you can be damn sure that I'm going to read all the relevant documentation and double/triple the invariants of any unsafe code I write in Rust (and maybe even get it reviewed by someone else). In some ways unsafe blocks in Rust are more unsafe than the equivalent C or C++ code because you have to uphold Rust's more stringent safety guarantees (e.g. no aliasing of mutable references), but they come with the saving grace that you don't need them very often, so you can afford to really take your time and implement them thoroughly.

dundarious 6y ago
Maybe that's all true, but in the specific example in question, he just does a string conversion and calls into a C library via ffi (for a reason he has justified). I think he has a real point about how the compiler should find this error if Rust is to fully match its marketing, so to speak. Clippy (a tool for style issues as much as anything else) doesn't seem to be an appropriate place for such error checking. On balance Rust does a great job, but "be even more careful in unsafe Rust than in regular C++" is probably a losing battle.
cesarb 6y ago
> From my perspective as someone who knows Rust but primarily works with high-level languages like JavaScript, some developers who from a background of unsafe languages (particularly C and C++) seem incredibly cavalier around `unsafe` blocks in Rust code. It's like they're desensitised to the unsafety. That's true. If you are used to 100% of your code being within the equivalent of a Rust "unsafe" block, having over 90% of your code being verified as safe by the compiler is a luxury. Even if one in ten lines are marked as "unsafe", that's already many times less "unsafe" than what they are used to in their C or C++ code. Moreover, C and C++ developers are used to all kinds of bizarre memory-saving and cycle-counting code that require "unsafe" in Rust. Things like intrusive double-linked circular linked lists, stashing things in the lower bits of pointers (I mean, it's a pointer to a 32-bit value which will always be 32-bit aligned, the lowest two bits will always be zero, why not use them?), doing XOR on pointers, and plenty of other useful tricks.

joosters 6y ago

Rendering a web page requires an in-page animated progress bar now?

skrebbel 6y ago
Only if you have JavaScript enabled.
horsawlarway 6y ago
And restarts the whole process every time you change focus away from the window (at least from chrome on win10)
- frosted-flakes 6y ago
  And also loses your scroll position.
camgunz 6y ago
It's just whiz-bang, doesn't actually do anything. I kinda like it!

howeyc 6y ago

Wait, what? I don't know rust, at all, but why is path freed? I thought rust didn't have GC? > let path = CString::new(filename.as_str()).unwrap().as_ptr(); Is one of those functions calling free behind the scenes?

deleted 6y ago
[deleted]
OskarS 6y ago
I mean, yeah, at the end you only get a raw pointer, the object managing the pointer doesn't exist anymore. I'm not a Rust programmer, but I would imagine it's equivalent to the following C++ code auto ptr = std::string(something).c_str(); This creates a std::string, calls c_str() on it to get the pointer. However, since the std::string isn't stored anywhere, its lifetime stops and it is destructed (this is why these are called temporaries), and the pointer is now invalid. The right way of doing it is this: std::string str { something }; auto ptr = str.c_str(); Now, str and ptr both live in the same scope, and ptr's lifetime is entirely contained in str's.
remexre 6y ago
like C++, rust frees values once they've gone out of scope; the result of CString::new(filename.as_str()).unwrap() is a temporary which is freed at the end of that line, since the .as_ptr() returns a non-lifetime-encumbered value.
cesarb 6y ago
It's freed for the same reason it would be freed in C++. Like C++, Rust relies heavily on RAII, in which resources are freed once they get out of scope. The "CString::new(filename.as_str()).unwrap()" returns a CString, which owns the memory for the C string. Since it's a temporary (it's not being stored anywhere), its scope ends before the next line, so the resources it owns are freed (by calling its Drop implementation) just after the ".as_ptr()" call. One solution would be to split it into two lines: let path = CString::new(filename.as_str()).unwrap(); let path = path.as_ptr(); That way, the CString would only release its resources at the end of the block. With references, the borrow checker doesn't let you do it the wrong way; however, the borrow checker only applies to references, not to raw pointers (which is what .as_ptr() returns).

brundolf 6y ago

> I used it for a while, but generally found it too tiresome. Worth noting that you can disable individual Clippy warnings instead of the whole thing (per line or for the whole project). I almost always turn off the dead_code warning, especially when I'm iterating

rrss 6y ago

> If there's an error which is a clear cut bug, I think it should be reported by an error detecting tool, not a linter Relevant rustc (merged) PR: https://github.com/rust-lang/rust/pull/75671 https://github.com/rust-lang/rust/pull/75671 - "Uplift temporary-cstring-as-ptr lint from clippy into rustc"

rectang 6y ago
This seems like a positive development, but there are other `as_ptr` and `as_mut_ptr` functions. The one that I tripped up with was actually from Vec, not CString. Zooming out, there are innumerable ways to create a dangling pointer. This is really a vexing problem.
- Jweb_Guru 6y ago
  Rust doesn't intend to prevent you from creating dangling pointers... if you want that functionality, use references. The reason this issue is particularly likely to hit is that it's the intersection of three things: (1) one of the very few times when people often need raw pointers when not writing very carefully inspected unsafe code is when calling functions across an FFI boundary, (2) C strings are represented by a char * pointer using a type (3) a Rust value that's created as a temporary will drop on the same line. (1) and (2) are how people can know it's almost always a bug when someone does this with a `CString`, whereas a lint would probably have a lot more false positives for something like a `Vec` (which is rarely passed to C directly since it doesn't understand it). Keep in mind that even something like borrowing a RefCell creates a temporary, so once you cast to a pointer and end the lifetime it came from it's very hard for the type system to track back the pointer you got to any particular deallocated temporary in an intelligent way. It pretty much has to be done on a case by case basis, I suspect (but maybe that could be improved--it is definitely the case, from a study someone did recently, that a large percentage of UB in unsafe Rust is due to destructors running early unexpectedly!).

rectang 6y ago

In Rust, dereferencing a pointer is unsafe and so must be isolated within an `unsafe` block — but creating a dangling pointer is not unsafe. What this means is that as soon as you add `unsafe` code which accepts a pointer argument, all the supposedly "safe" code which can affect this pointer argument becomes a potential source of memory errors. To me, this was unintuitive — I expected the `unsafe` block to require an extra level of scrutiny, not the surrounding code. After making an error similar to the one described in this post, I wondered, "why isn't creating a dangling pointer unsafe?" But after following that train of thought I realized that vast amounts of code would be pulled into `unsafe` blocks as a consequence, so it's not a viable approach — and thus it became apparent why only dereferencing is unsafe. The lesson seems to be that dereferencing requires extreme care, but I wish potential errors weren't so subtle. The `as_ptr` family of functions has been tripping people up for a long time: https://users.rust-lang.org/t/cstring-as-ptr-is-incredibly-unsafe/11431 https://users.rust-lang.org/t/cstring-as-ptr-is-incredibly-u...

stefan_ 6y ago
In general unsafe should be viral: you can't call unsafe without yourself being unsafe. But it hits the same "snag", then nothing works anymore.
- nynx 6y ago
  This is not true. Rust is based around the idea of encapsulating unsafety within interfaces that are safe to use.
- Jweb_Guru 6y ago
  If `unsafe` were viral it would be useless. Syntactically ill-typed code can still be proven semantically well-typed, and `unsafe` is intended as a marker that the proof of semantic soundness lies outside of Rust's type system. If there is no proof (at least an informal one), you shouldn't be using unsafe, generally speaking.
jdc 6y ago
I wonder what a good approach to safe pointer arithmetic would be.

mqus 6y ago

I'm somewhat wishing for a `.bind!()` macro or something similar, which will desugar `let y = x.method().bind!().reference_and_do_something()` into `let tmp = x.method(); let y = tmp.reference_and_do_something()` . This should be available for chaining for convenience. But I see the issues (e.g. which scope should `tmp` be bound to/what is its lifetime?)

nyanpasu64 6y ago

Fun fact: I hit the exact same bug when working on FamiTracker in C++, with a MFC type also called CString... "construct a CString, acquire a pointer into the internals, destroy the CString". The only reason it doesn't make FamiTracker crash is because MFC's CString is COW/refcounted, and the refcount never reached zero because it was constructed from a long-lasting string literal, so the underlying buffer was still valid after destroying the string object. I feel that tying deallocations to destructors is a benefit to safe code, and a hindrance to unsafe code. I feel explicit calls to "destructor" functions that move self (perhaps using defer), plus linear typing to statically ensure you don't forget to call a destructor or call one twice, can provide safety in both situations, but at the expense of verbosity and not interacting well with exceptions. I still believe linear typing (argued against by Gankra in https://gankra.github.io/blah/linear-rust/ https://gankra.github.io/blah/linear-rust/) will fix multiple deficiencies in Rust: - It helps write allocation-free code which passes objects around without creating/destroying them (maybe an Alloc effect would help too). - Making drops explicit makes it harder to screw up when writing unsafe code performing allocations/deallocations. It may or may not help protect against double-frees that occur upon panicking. - File's Drop implementation ignores the return code of fclose(), since you can't return values from a destructor. So dropping a File directly should be prohibited (except during panic-unwinding, which I feel is a rare scenario where perfect error-checking is less crucial than not risking more panics). Instead you move the file into a close() method/function, and are expected to check the Result<(), error> that comes out. - From what I've read, trying to perform an asynchronous cancellation operation in the standard Drop method is not possible, since the method lacks access to the executor (unless you hard-code an executor to block on). It would make sense to make explicit dropping illegal (as a lint) and require moving it into a "drop"-like function, but keep Drop around to be called during panic-unwinding. This may result in logically incorrect behavior, but is still memory-safe. And I think it's better than making your async-fn state machines always larger to fit awaiting-a-drop states, and awaiting the async runtime when you panic-unwind.

rectang 6y ago
> File's Drop implementation ignores the return code of fclose() Yeah, and it bothers me that I can't call `close` and check for errors. All you need is something like this: pub fn close(self) -> io::Result<()> { let status = unsafe { libc::close(self.fd) }; mem::forget(self); // prevent drop and invalid double-close match status { 0 => Ok(()), _ => Err(io::Error::new(ErrorKind::Other)), } } Note that this function takes `self` not `&mut self`, so it consumes the resource and prevents further use. Then `mem::forget` prevents `drop` from running. See discussion at https://github.com/rust-lang/rust/issues/59567 https://github.com/rust-lang/rust/issues/59567

swader999 6y ago

This guy writes well and codes dangerously. Would have a beer with.

cryptonector 6y ago

What a great blog post. This happens in garbage collected languages too. I've been bitten by this in Java (JNI). Yes, this needs to be caught by the compiler, damn it. And linters shouldn't complain about style either. Style checking should be a separate and very configurable tool.