Hacker News Reader

Comment by torstenvl | Hacker News Reader

I wouldn't quite say this is bad advice, but it isn't necessarily good advice either.

I think it's somewhat telling that the chosen language is Rust. The strong type system prevents a lot of defensive programming required in other languages. 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. So at least some `if`s should definitely be down (preferably in a way where errors bubble up well).

I feel less strongly about `for`s, but the fact that array arguments decay to pointers in C also makes me think that iteration should be up, not down. I can reliably know the length of an array in its originating function, but not in a function to which I pass it as an argument.

View on HN

> 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.

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.)

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.

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

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.

In C a NULL is often a valid non-dereferencable pointer value. If you're checking for invalid pointer values you need to check for any of the 0xfffffffffffffffe possible invalid values. If all you're doing is checking for NULL you're not checking for invalid values at all.

If the precondition of your function is "parameter p can not be NULL" that's fine, go ahead and check for it. If the precondition is "parameter p must be a valid pointer" well then, good luck to you finding the appropriate assertion condition.

My rule of thumb is: if the type system doesn't prevent an invalid value, it's your responsibility to prevent it at run-time.

I've been writing a lot of T-SQL lately, which doesn't let you declare a parameter or variable as NOT NULL. So it's a good idea to check for NULLs as early as reasonable - usually at the top of the stored procedure (for parameters). Otherwise, a NULL might propagate unexpectedly deep into the call hierarchy and cause less-than-obvious problems.

Fortunately, the data in the table can be declared as NOT NULL, so these kinds of bugs will usually not corrupt the data, but catching them as early as possible makes life easier. However, if there is piece of logic that writes something to the database depending on the value of some parameter, and that parameter is unexpectedly NULL, that might lead to a wrong thing being written, or a necessary thing not being written at all, effectively corrupting the data.

So, defensive programming all the way, baby!

Reply on news.ycombinator.com