“(null)” is a bad idea

http://www.daemonology.net/blog/2015-08-13-null-is-a-bad-idea.html

by cperciva • 11 years ago

60 73 11 years ago

vortico• 11 years ago

In the context of where this would be a bad idea (e.g. flight control, rescue rover, etc), you would not use `printf()`, or perhaps any standard output at all. If anything, you will use lower level `write()`, where EFAULT or something would be raised on NULL, and/or static buffers, where NULL is impossible. `printf()` is really only good for developer-friendly programming, where absolute rigor is not the focus.

cperciva12• 11 years ago

`printf()` is really only good for developer-friendly programming, where absolute rigor is not the focus.

printf gets used all the time in systems code. I count 1221 occurrences of printf (and friends) in BIND 9 alone.

More to the point, "(null)" was just an example of a debugging-hostile pro-vulnerability attempt to silently "fix" bad data.

jeremiep8• 11 years ago

BIND 9 doesn't have the same requirements as flight control software. Nobody will die if BIND segfaults, at least I hope not.

The software running in your car, plane, spacecraft and the likes have very strict regulations and you will find it radically different from systems code found in a linux distribution, even if written in the same language!

See http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf for more details. I wouldn't want to write code like this as it sounds very unproductive, but their reliability and astronomically small number of bugs speaks for itself.

setpatchaddress2• 11 years ago

Venerable Unix utilities are generally not paragons of good modern systems programming practice.

SamReidHughes• 11 years ago

With compiler-checked format strings (the norm today) printf is just fine.

tbrownaw3• 11 years ago

anti-pattern known sometimes as "defensive programming": If something goes wrong, pretend you didn't notice and try to keep going anyway

No. Defensive programming means, be predictable even in the face of nonsense. Return an error code instead of segfaulting, define reasonable non-error behavior for thing that don't have to be errors, etc. Sweeping things under the rug just means they'll come back to bite you later, which isn't defensive at all.

and when faced by a choice between making the presence of a bug immediately obvious or trying to hide the bug and potentially exposing serious security vulnerabilities... well, I know which I would prefer

This over a feature that makes "printf debugging" more usable? Is he smoking something?

If the security of your code relies on a parameter not being null, you'd be a fucking idiot to let "printf() should segfault" be the way you catch that. Check it explicitly; not only is it safer but it makes broken callers easier to debug.

cperciva12• 11 years ago

Sweeping things under the rug just means they'll come back to bite you later, which isn't defensive at all.

I did say "known sometimes as". And I've seen a lot of "check for NULL and silently replace it by something reasonable so that bugs don't make us crash" code justified as being "defensive".

If the security of your code relies on a parameter not being null

That's not what I said. Exploit paths may however result in pointers being NULL which are normally non-NULL, and so replacing NULL by "(null)" may turn a denial of service into something worse.

Check it explicitly

I absolutely agree, developers should use more assertions.

tbrownaw3• 11 years ago

That's not what I said. Exploit paths may however result in pointers being NULL which are normally non-NULL, and so replacing NULL by "(null)" may turn a denial of service into something worse.

Exploit paths will likely cause variables to have values outside their expected range.

Therefore library functions should be defined as narrowly as possible, in order to convert as many as possible of these cases into crashes.

Never mind that this makes the library functions less useful.

Never mind that this will push complexity into the caller (or just flat-out add complexity in some cases). Complexity which will necessarily add more opportunities for bugs.

skynetv2• 11 years ago

The behavior is bad because thats not what I expected it to do, so it must be bad.

kind of an unreasonable on the authors part

cperciva12• 11 years ago

not what I expected it to do, so it must be bad.

Not at all what I said. What I was saying was "actively prevents bugs from being found and fixed, so it must be bad".

delan• 11 years ago

It can be difficult to let go of printf(3) debugging, but using a debugger instead can seriously improve your development workflow.

jeremiep8• 11 years ago

Not for multi-threaded or networked code where breaking in the debugger changes the program's behaviour and causes timeouts. I still prefer printf in these cases.

tbrownaw3• 11 years ago

And in situations where it's applicable, I'm perfectly happy to use a debugger.

Unfortunately it doesn't work so well for things like intermittent wrong behavior in long-running programs. Better in that case to use pervasive logging (aka, printf debugging to a file).

ikeboy• 11 years ago

So is "is a bad idea" replacing "considered harmful"?

cperciva12• 11 years ago

Huh. Somehow the words "considered harmful" never even occurred to me while I was writing that.

rebekah-aimee• 11 years ago

The phrase "considered harmful" is considered harmful, remember?

http://www.catb.org/jargon/html/C/considered-harmful.html

millstone2• 11 years ago

I often use printf for debugging. An unexpectedly NULL string is exactly the sort of thing I want it to help me debug!

cperciva12• 11 years ago

Crashing works just as well as printing "(null)" for that purpose. Possibly even better, since you'll have a core dump. ;-)

millstone2• 11 years ago

Sometimes it does, but often it does not:

    printf("name: %s %s", first, last);

If it crashes, there's no easy way to know which one was NULL!

kazinator10• 11 years ago

Crashing can interefere with the debugging session. You have the program stopped at a breakpoint and tell gdb to run some function inside the program which prints something with printf. Oops, that function blows up! The whole thing is foobared now; you've injected a fatal signal into the program that wasn't there.

nitrogen4• 11 years ago

A note for those encountering difficulty: most people will have to type ulimit -c unlimited to get core dumps.

    $ echo 'int main(void){ return *(int*)0; }' > crash.c
    $ gcc -Wall -Wextra -std=c1x crash.c -o crash
    $ ./crash
    Segmentation fault
    $ ulimit -c unlimited
    $ ./crash
    Segmentation fault (core dumped)

x0x0• 11 years ago

    fprintf(stderr, "test %s\n", s != NULL ? s : "NULL");
    if(s == NULL) abort();

kazinator10• 11 years ago

This is nonsense. There is no reason why printf has to be the sacrificial function that blows up on null. Those same "misguided" operating systems run on hardware that traps null dereferences via an unmapped page. A predictable, tidy crash is just around the corner if that same is used in the program. On the other hand, if that printf is the only place the pointer is used, then there is no security hole. It's just undefined behavior on ISO paper, and you don't agree with how it is locally defined by the implementation.

How do we know it's a security hole? Maybe some structure has a "char * " name field, and null indicates "anonymous/has no name". It's an ISO C conformance bug to try to print that with %s, but that's a portability issue. If that's the only problem in the program (everywhere else it checks for null), there is no real issue.

POSIX and C libraries are not entirely about allowing null pointers or not. For instance gettimeofday lets you pass a null pointer for the "struct timezone * " second argument, which indicates that you aren't interested in specifying or retrieving it. On the other hand, a null argument is undefined behavior in getgroups, even if the array size is specified as zero (meaning, return me the size of the array actually needed and I will call you again soon with such an array)! A non-null pointer must be passed as a POSIX-conforming eggcorn. According to the logic of this article, if I pass null I'm committing a security hole, and an OS which allows getgroups(0, NULL) is misguidedly sweeping it under the rug.

Null pointers are not some "bogey man". I built a Lisp dialect in whose implementation a C null pointer represents the nil symbol (empty list, false, ...). It prints as "nil", satsifies the symbolp function and so fort. Lexical variables are initialized to nil if not given an initial expression. Some C library printing a null pointer as (null) seems like nothing by comparison.

Be that all as it may, you cannot actually fix this perceived issue by changing from (null) to crash, cold turkey. Unknown numbers of nonportable programs out there (otherwise well-tested programs) which rely on this will just crash.

I just did a "grep -r -F '(null)' /var/log" on a system here and found a few hits. Most of those are probably via the %s conversion, and all that code would have to be fixed if printf were to assert this. And that's just instances confined to logging, not all output everywhere.

cperciva12• 11 years ago

There is no reason why printf has to be the sacrificial function that blows up on null.

Of course not. Every function which has undefined behaviour upon being passed NULL should blow up on NULL.

In general, anything which can crash should crash, because that's the fastest way to make sure that bugs get found.

kazinator10• 11 years ago

printf doesn't have undefined behavior on being passed null.

  printf("%p\n", (void *) 0); // not undefined

It's the specific conversion specifier %s which doesn't have defined behavior.

anything which can crash should crash

__asm__ __volatile__ can crash. It's undefined behavior.

rtpg3• 11 years ago

Maybe some structure has a "char * " name field, and null indicates "anonymous/has no name". It's an ISO C conformance bug to try to print that with %s, but that's a portability issue. If that's the only problem in the program (everywhere else it checks for null), there is no real issue.

I'll take 'uses of Option/Maybe' for 200 Alex

Some might say it's overkill, but if you can have a char* be null, then for most intents and purposes you have two states to deal with on any usage of that value.

Just because such a design can exist doesn't mean we should hobble our tools to support it.

kazinator10• 11 years ago

It could simplify things elsewhere.

In Lisp, CAR and CDR accessors take a cons cell or NIL, which isn't a cons cell, and has no such fields.

It wasn't always like that; according to the HOPL paper on evolution of Lisp, InterLisp introduced this nicety and it was copied by others.

These functions are complicated with that extra state, but programs are simplified.

A null "char *" indicating "this object has no name" is better than some special string value doing the same, because it's different from every possible string value.

dllthomas2• 11 years ago

A nullable char pointer is a perfectly sensible implementation of "Maybe String".

At least, presuming a non-nullable char pointer is a perfectly sensible implementation of String.

linkydinkandyou• 11 years ago

I think it's a Bad Idea in C/C++, but it may not be a Bad Idea in some untyped interpreted languages, or in a special "checked build / debug" C++ environment.

jeffdavis2• 11 years ago

"This is an example of an anti-pattern known sometimes as "defensive programming": If something goes wrong, pretend you didn't notice and try to keep going anyway."

That's why SQL NULL is also a bad idea.

tytron• 11 years ago

How else would you represent missing data in SQL than using nulls?

wvenable• 11 years ago

SQL NULLs propagate; you can't get an answer from a NULL unless you explicitly plan for it.

nitrogen4• 11 years ago

It can be a bit counterintuitive that "([NULL] < x)" and "NOT ([NULL] < x)" both return NULL in SQL.

    MariaDB [(none)]> select null as a union all
        select 1 as a;
    +------+
    | a    |
    +------+
    | NULL |
    |    1 |
    +------+
    2 rows in set (0.01 sec)
    
    MariaDB [(none)]> select * from (select null as a
        union all select 1 as a) T where a >= 1;
    +------+
    | a    |
    +------+
    |    1 |
    +------+
    1 row in set (0.00 sec)
    
    MariaDB [(none)]> select * from (select null as a
        union all select 1 as a) T where NOT a >= 1;
    Empty set (0.00 sec)

jeffdavis2• 11 years ago

That's false.

SELECT SUM(x) FROM foo;

If some x values are NULL and others not, you will never know.

__david__• 11 years ago

Having "%s" print "(null)" is perfectly fine for debugging and actually makes your life a lot easier since seeing "(null)" in your output is much quicker than popping into the debugger trying to figure out what line caused a segfault. (Yes, you can do it with a ternery but writing rigorous debug code is never on anyone's list).

Having "%s" print "(null)" is absolutely useless in the meat of your string manipulation code. It's pretty much guaranteed to be a bug and could easily mask a problem (as described in the article).

So I think the real problem here is that printf() is used for both debugging and for non-debugging string manipulation (sprintf() and friends). It's trying to serve two masters and it can never get both cases right. Maybe we should make "%s" not check for null and add a null-able "%S" for debugging.

pjtr• 11 years ago

"If you accidentally pass a null pointer as the argument for a ‘%s’ conversion, the GNU C Library prints it as ‘(null)’. We think this is more useful than crashing. But it’s not good practice to pass a null argument intentionally."

https://www.gnu.org/software/libc/manual/html_node/Other-Out...

It's documented.

cperciva12• 11 years ago

There are more C libraries than just GNU C...

kazinator10• 11 years ago

If there weren't, we wouldn't need a library clause in the ISO C standard to know what is portable among them. That doesn't add up to an argument that libraries shouldn't have documented extensions in areas left undefined by the standard.

kazinator10• 11 years ago

From BSD errno manpage:

     14	EFAULT Bad address.  The system	detected an invalid address in
	       attempting to use an argument of a call.

Is that an example of a misguided operating system? The kernel should kill the offending process instead of sweeping the bug under the rug with an errno code that might not be examined.

Those who live in glass houses ...

the_why_of_y2• 11 years ago

I have not checked which syscall or libc function could return that, but it's quite possible that some applicable standard like C or POSIX or SUS mandates such a return value.

It is better to follow the standard in such cases, so that portable code written against the standard runs nicely.

However (claims the OP) in the case of printf, the %s null pointer case is explicitly defined to be undefined behavior and hence the implementation should handle this either implicitly (by crashing with null pointer dereference) or explicitly by calling abort(), as that will teach programmers not to rely on superficially benign implementations of undefined behavior.

kazinator10• 11 years ago

mandates such a return value.

And ISO and IEEE standards can't be "misguided", just competing operating systems that are more successful.

explicitly defined to be undefined behavior

Contradiction.

Undefined behavior is not defined, explicitly or otherwise. There can be explicit wording that some requirement is not being made.

When ISO C explicitly states that there is an absence of a requirement, that does not carry any more or less weight then when behavior is undefined by omission of any wording of a requirement. All UB is equal.

not to rely on superficially benign implementations of undefined behavior.

An excellent example of beneficially fixing undefined behavior is to implement strict left to right ordering on all operations and their side effects.

A language or language dialect in which evaluation order is defined is safer and better for engineering.

gpvos• 11 years ago

"on some misguided operating systems" — on which well-thinking systems does printf crash in this case?

“(null)” is a bad idea

Related Stories