Comment by tptacek | Hacker News Reader

Comment by tptacek | Hacker News Reader

Parent Full thread

Ehh, sort of. This is a NULL pointer read; it's a crasher that can't be further weaponized. Approximately the same thing happens in Python, Javascript, Java, and Go when you mis-handle a nil value.

Rust goes through some trouble to avoid nil values altogether, and it's great. But in practice, applying matching and iflets to every return value everywhere makes for very noisy code (I mean, Rust is already very noisy, but bear with me), so the idiom in the language is to call "unwrap" on Option and Result values that are "known" to be safe. A mishandled "unwrap" will do approximately the same thing to your program as dereferencing a NULL pointer will.

Rust helps here a lot, more than most other languages. It does not foreclose on this kind of bug, though, the way it (and other memory safe languages) foreclose on other memory corruption vulnerabilities.

(This is notable not the case for NULL pointer _writes_, which occasionally can be weaponized. Use Rust in preference to C.)

Dereferencing null is UB; there is no guarantee that it will compile to something that segfaults or that it won't be exploitable. This is unlike None.unwrap(), which is a guaranteed panic.

Exploitable null dereference, for anyone who needs a reminder: https://lwn.net/Articles/342330/

The example you've provided isn't simply a NULL pointer dereference. The attacker had control over memory mapping! NULL pointers can (uncommonly) be exploitable --- especially in the kernel, where the 0 address can be mapped --- but you can't generally exploit them simply by attempting to read from them. The most common general pattern I'm aware of is a write, through a NULL pointer, that includes an unbounded offset.

This isn't that.

`?` would be the natural thing to do in Rust, rather than unwrap.

I know that's the case, but I looked a bunch of Rust codebases that I had lying around (exa, Firecracker, netlink, Servo), and I see a ton of `unwrap`s, too; not just in test code.

Yeah, I made this mistake in trust-dns. Went back and thought better on it, and replaced all unwraps with proper checks. After that no more random crashes, but you're correct that it's easy even in Rust to say, "I'm sure this is never None in this case", and be very wrong.

The Rust linter (clippy) does perform a bunch of checks on unwrap usage now. For example, a common mistake before TryFrom was added to the stdlib was to implement From between two types and do an unwrap on conversion (think String -> enum variant), and clippy will now suggest TryFrom as a replacement since the unwrap is hiding a fallible case.

The fact that folks use unwrap so much in tests and examples can also mislead people new to the language that this is a common practice.

I thought about that --- particularly for Firecracker, which has so much test code --- and no, it's in the actual code too.

I think Rust does more than any other mainstream language to mitigate this problem. I'm just saying, it still exists in Rust; it's just called a panic on unwrap/expect, instead of "null pointer exception".

Using unwraps is often the “proper check”. Trying to come up with error handling in cases where invariants are not met is usually done wrong.

That's not a check at that point, though, it's an assertion that the None case is invalid.

I'm not saying you should never use unwrap, but I've been burned by it when treating it a little too nonchalantly. If you're implementing a lower level library, like a dns stub resolver, panicking will bring down the software using your library... which is definitely not something people are generally happy about.

Error handling isn't always the right thing, sometimes just returning Option is the correct thing.

Yeah it may be valid, barring careful consideration of the exact situation. My canonical example is a web server.

In the context of handling a web request, never unwrap, because a web server should never die no matter what kind of weird or crazy thing a client somewhere sends it. Check every error for everything in that context, always at the very least drop back to a code path that returns a 500 and keeps the server running.

In the context of server init though, unwrap is good. If anything in the server init process goes wrong, that means I as the developer / deployment engineer did something wrong. The server should then blow up to let me know that something is very wrong and needs fixing. The server definitely should not try to rattle to life anyways and run in some sort of degraded state where it can't handle requests correctly.

Usually `.expect` is better, as you can explain the error.

Reply on news.ycombinator.com