Hacker News Reader

Comment by lytigas | Hacker News Reader

> A C programmer who doesn't check the validity of pointers passed to functions and subsequently causes a NULL dereference is not a C programmer I want on my team.

I disagree. Interfaces in C need to carefully document their expectations and do exactly that amount of checking, not more. Documentation should replace a strong type system, not runtime checks. Code filled with NULL checks and other defensive maneuvers is far less readable. You could argue for more defensive checking at a library boundary, and this is exactly what the article pushes for: push these checks up.

Security-critical code may be different, but in most cases an accidental NULL dereference is fine and will be caught by tests, sanitizers, or fuzzing.

View on HN

I agree with that. If a function "can't" be called with a null pointer, but is, that's a very interesting bug that should expose itself as quickly as possible. It is likely hiding a different and more difficult to detect bug.

Checking for null in every function is a pattern you get into when the codebase violates so many internal invariants so regularly that it can't function without the null checks. But this is hiding careless design and implementation, which is going to be an even bigger problem to grapple with than random crashes as the codebase evolves.

Ultimately, if your problem today is that your program crashes, your problem tomorrow will be that it returns incorrect results. What's easier for your monitoring system to detect, a crashed program, or days of returning the wrong answer 1% of the time? The latter is really scary, depending on the program is supposed to do. Charge the wrong credit card, grant access when something should be private, etc. Those have much worse consequences than downtime. (Of course, crashing on user data is a denial of service attack, so you can't really do either. To really win the programming game, you have to return correct results AND not crash all the time.)

Yeah but, not checking for null in C can cause undefined behavior. One possible outcome of undefined behavior is that your program doesn't even crash, but rather continues running in a weird state. So such a bug doesn't always "expose itself".

If we accept that bugs are inevitable, and that accidentally passing a null pointer to a function is a possible bug, then we also conclude that your code really should include non-null assertions that intentionally abort() the program. (Which run in debug/staging mode but can be disabled in release/production mode.)

Very good point. For C, I like the idea of sticking an assertion in there.

Indeed, Rust's own standard library uses this method. There are lots of public-facing unsafe functions that can result in undefined behavior if called incorrectly. But if the standard library is compiled in debug mode (which currently requires the unstable flag -Zbuild-std), then it will activate assertions on many of these unsafe functions, so that they will print a message and abort the program if they detect invalid input.

The Rust compiler has even started recently to put extra checks on unsafe code in codegen, e.g. on raw pointer dereference to check it is aligned.

That raises a more general point. When you can't or don't have compile-time checks, removing run-time checks in production amounts to wearing your seat belt only when driving around a parking lot and then unbuckling when you get on the highway. It's very much the Wrong Thing.

I wouldn't really characterize it that way. You (ideally) shouldn't be hitting code paths in production that you didn't ever hit in testing.

But, in any case, if you are fine with the slight performance hit (though many C/C++ projects are not), you can always just keep assertions enabled in production.

assert_always(ptr != nullptr);

(custom assert_always macro, so it doesn't get compiled out in release builds)

I used to ask this same question in interviews: should C code always check for NULL? My favorite answer was that the code should have a notion of boundaries, runtime checks should happen at the boundaries, and debug-only assertions are nice but not required inside the boundaries.

Reply on news.ycombinator.com