Comparative unsafety

https://flak.tedunangst.com/post/comparative-unsafety

by lionkor • 5 years ago

52 93 5 years ago

flak.tedunangst.com

einpoklum4• 5 years 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.

thaumasiotes4• 5 years 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.

im3w1l2• 5 years 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?

ncmncm4• 5 years 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.

MaulingMonkey2• 5 years ago

Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done.

malloc is worth null-checking, if only to assert/panic/breakpoint/fatal early and cleanly.

Even on linux with overcommit, it'll return null on memory space exhaustion, which can easily happen even in a 64-bit processes if someone feeds your program maliciously crafted data with an oddball size. A crash dump at the point of allocation failure, when the sizes are still on the call stack / in registers, instead of minutes later - when the pointer is actually used - is also a vastly better debugging experience when fuzzing for such bugs.

If malicious data can control offsets into the (null) pointer, an allocation failure can turn into an exploitable buffer overflow (just make the offset large enough), even if the offset is bounds-checked against the "expected" (even larger) allocation size.

einpoklum4• 5 years ago

Everyone nods and say you should, and then keep writing code without checks,

Well, I would take points off for not checking return values, although TBH my basic course did not have file I/O (they would use input and output redirection instead, to keep the programs focused on more fundamental things).

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?

That's a trickier subject, because it's not just the hassle of checking return values, it may mean your whole approach to implementing something may go down the drain. Also, it's much less obvious; and compilers don't make an effort to warn you about it; and telling the difference between _undefined_ and _implementation-defined_ behavior is an art which few master (and I'm not even one of those few).

This is entertaining to watch regarding UB:

CppCon 2017: Piotr Padlewski “Undefined Behaviour is awesome!”

https://www.youtube.com/watch?v=ehyHyAIa5so

marvel_boy• 5 years ago

Teaching mode requires the stove be preheated to the optimal temperature.

Very poetic. Are you Russian?

thaumasiotes4• 5 years 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.)

adwn2• 5 years 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.

ChrisSD4• 5 years 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.

thaumasiotes4• 5 years 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".

boardwaalk• 5 years ago

If your reaction to Rust actually following the prototype of chmod but not accepting conversions signed/unsigned silently is to redefine chmod so you can do it like you’d do in C… that seems a little bullheaded to me. Wrapping a C function is dangerous in a literal and real sense, doing “-1 as u32” is not at all.

thaumasiotes4• 5 years ago

If your reaction to Rust actually following the prototype of chmod but not accepting conversions

Eh... as the article points out, the chown documentation specifically says that you are expected to provide negative numbers. The spec is obviously bugged.

This limits the amount of righteousness you can claim over "but we followed the spec (that we knew was wrong)!"

NovemberWhiskey• 5 years ago

If you read the POSIX specification for chown you'll see that the "leave unchanged" sentinel value is, for UIDs, defined to be (uid_t) -1.

I can't be bothered to try it myself but I'm fairly sure the author could just have written (-1i32) as uid_t and gotten some of their life back.

cryptonector4• 5 years ago

Yes. Specifically, POSIX does not specify whether id_t, uid_t, gid_t, or pid_t are signed or unsigned, so whenever it refers to the "default"/"leave alone"/"unspecified" value, it always does it as a cast of -1.

cryptonector4• 5 years ago

Although `~0` assumes twos-complement signed numbers. Which, well, is probably a safe assumption, but still.

IshKebab• 5 years ago

All modern computers use twos-complement and modern languages like Rust require it. Not assuming twos-complement is a theoretical nitpick along the lines of not assuming a byte is 8 bits. Nobody does that.

AndrewDucker• 5 years 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!

nicoburns2• 5 years 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.

tyingq3• 5 years ago

Yep.

  window.setTimeout(makeprogress, delay)

With various repeated hard-coded delays of 50, 250, and 500ms.

tyingq3• 5 years ago

Extra maddening, as it works fine with Javascript disabled. Those loading and rendering progress bars are a terrible idea.

stefan_2• 5 years ago

Haha, it's true! Disabling JavaScript leaves a perfectly working article. Is this one big troll?

caslon2• 5 years ago

It's a joke. It's not a terrible idea. It's a great idea! A great joke!

chriszhang• 5 years ago

If I try to search something within the page with Ctrl + F that also causes the whole page to re-render and display "loading ..." and "rendering ..." progress bar.

liminal• 5 years ago

You can't even search within the page. Ctrl-F hides the content and shows the progress bars.

ptomato• 5 years ago

I'm pretty sure it's explicitly intended solely as punishment for people who have javascript enabled.

skrebbel3• 5 years ago

Fwiw I think it's pretty hilarious.

Also I warmly recommend to view-source, the makeprogress() function is a true delight.

GuB-42• 5 years ago

Looking at the source, the progress bar is completely fake, it really does nothing besides making you wait. It even introduces slight delays to make it look more realistic.

Why? Just why? The worst part is that the site is actually very light, with none of the ads, analytics and resource hog frameworks that are all too common these days. It probably could load almost instantly if it wasn't for that fake progress bar.

Edit: Judging by the other comments, it looks like it is some kind of political statement against Javascript. I disagree with such practices but well, that's his site, his choice.

ChrisSD4• 5 years ago

It's meant as satire on the needlessly script heavy websites that many people create nowadays. The blog post itself isn't entirely serious either.

wyldfire• 5 years 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.

pornel2• 5 years 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

nicoburns2• 5 years 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• 5 years 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.

cesarb3• 5 years 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.

nyanpasu644• 5 years ago

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.

As a Rust programmer who's written safe code and some unsafe abstractions, one in ten lines being unsafe means that the remaining 90% of your lines need to be written with the same care as your unsafe code, to avoid feeding invalid arguments into unsafe operations which cause UB. The general approach is to not scatter unsafe code around your entire program, but segregate it into safe abstractions that prevent callers from triggering UB. (Though some APIs make it difficult to achieve, so the alternative is to create unsafe abstractions that expect callers to take care to avoid triggering UB, which requires auditing the callers for security as well.)

joosters• 5 years ago

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

skrebbel3• 5 years ago

Only if you have JavaScript enabled.

horsawlarway• 5 years ago

And restarts the whole process every time you change focus away from the window (at least from chrome on win10)

frosted-flakes• 5 years ago

And also loses your scroll position.

camgunz• 5 years ago

It's just whiz-bang, doesn't actually do anything. I kinda like it!

howeyc• 5 years 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?

OskarS2• 5 years 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• 5 years 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.

cesarb3• 5 years 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).

iainmerrick2• 5 years ago

So shouldn’t .as_ptr() itself be marked unsafe? That way only the correct version of this code, with the .as_ptr() inside the unsafe block, would compile, right?

(I assume there’s a good reason why this isn’t the case, but I don’t know Rust well enough to know the answer. I’d be interested to learn, if anyone can explain it for me!)

Edit to clarify: what seems very strange to me is that a function that not only does something potentially unsafe, but returns an unsafe value would be callable outside of an “unsafe” block.

mqus2• 5 years ago

to be clear, not path is freed, but what path points to, meaning the result of CString::new(...).unwrap()

Because it is not bound to a variable, it will get dropped before the next line, even if we create a pointer to it. if you would have borrowed instead of creating a pointer this would have been a compiler error because the borrow would have outlived the original object. But a pointer exists outside of the borrow semantics and lifetimes, that is also why it can only be used inside the unsafe block.

cryptonector4• 5 years ago

It's freed for the same reason it could be freed in Java. In Java it could be freed immediately after as_ptr() returns if that was the last reference, but may be freed later. In Rust it would be freed immediately after as_ptr() returns because that was the last reference.

brundolf• 5 years 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• 5 years 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 - "Uplift temporary-cstring-as-ptr lint from clippy into rustc"

rectang7• 5 years 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_Guru3• 5 years 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!).

rectang7• 5 years 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-u...

stefan_2• 5 years 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• 5 years ago

This is not true. Rust is based around the idea of encapsulating unsafety within interfaces that are safe to use.

Jweb_Guru3• 5 years 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• 5 years ago

I wonder what a good approach to safe pointer arithmetic would be.

pornel2• 5 years ago

Rust's actually-safe pointer arithmetic is done via slices (because it's necessary to track the length):

    slice = &slice[1..];

It compiles to roughly `ptr++; len--;` and a bounds check if necessary.

`1234 as *const u8` and `ptr.offset(n)` are also safe, because Rust allows invalid/dangling pointers to exist if they're never dereferenced.

mqus2• 5 years 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?)

nyanpasu644• 5 years 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/) 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.

rectang7• 5 years 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

verdagon• 5 years ago

I'm a big fan of this kind of improved RAII. We implemented it in https://vale.dev/blog/raii-next-steps, you might find it interesting!

nyanpasu644• 5 years ago

Read the blog, found it somewhat interesting. Prohibiting destructors and only allowing consuming methods is certainly easier when you don't have an exception system.

Bidirectional pointers is definitely something it does better than Rust. Constraint references are an interesting idea. Clasps remind me of some Qt objects automatically detaching other objects listening to them, when destroyed (eg. QSortFilterProxyModel's source model, referenced by QSortFilterProxyModel, referenced by QAbstractItemView), but it feels ad-hoc for every individual type, and I don't know if there's underlying principles that all Qt types follow.

The descriptions of the language and others feel a bit crank-ish at times, I got lost a web of hyperlinks to various pages on the site and didn't read all of them, and apparently the language is not ready yet. I might check it out again when it's more complete.

swader999• 5 years ago

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

cryptonector4• 5 years 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.

Comparative unsafety

Related Stories