Search: Home Bugtraq Vulnerabilities Mailing Lists Jobs Tools Vista
Digg this story   Add to del.icio.us  
Introduced: A resolution resolving the semantic quarrel over malloc checking.
Thomas Ptacek, Matasano 2008-04-18

This is all my fault.

Many moons ago, I wrote a blog post chiding developers for checking the return value of malloc, the C function that allocates chunks of memory for programs to work with. When malloc fails, it returns NULL. According to Hoyle, you’re meant to check for that value, because malloc can fail at absolutely any time (you are not the only program claiming memory).

I stand by that argument, and by most of the wording of that blog post. Now about that word “most”.

Dave LeBlanc and I go back, though he may not remember that. Last bubble, we were dev leads on competing products. We’ve taken different career paths, and, long story short, he’s technically now more of an authority on secure coding than I am. And I’m telling you this because LeBlanc’s response to my last post is —- faithful paraphrase —- “are you high?”

LeBlanc thinks you’d have to be not check malloc returns, because:

  • not checking will inevitably crash the program, and crashes are bad,

  • not checking leads to the bug class Dowd found, and

  • not checking leads to the bug class Dowd found.

LeBlanc is right. I am wrong. “Not checking” is bad. Let me make a very slight semantic adjustment, so that I might be inassailably correct (again).

Here are three (extremely contrived) code examples. The first, let’s call, “unchecked”. It simply doesn’t check the return of malloc.

#define hostile /**/

void *
_setup(unsigned hostile slot, unsigned hostile id) {
        u_int32_t *slots = malloc(SLOTS_SIZE);

        slots[slot] = id; // XXX write32 corruption

        return slots;
}

The second, we’ll call “caller-checks”. As you can see, it does.

#define hostile /**/

void *
_setup(unsigned hostile slot, unsigned hostile id) {
        u_int32_t *slots = NULL;

        if((slots = malloc(SLOTS_SIZE)))
                slots[slot] = id;

        return slots;
}

Now the third, which looks suspiciously like the first, we’ll “callee-checks”.

#define hostile /**/

void *
_setup(unsigned hostile slot, unsigned hostile id) {
        u_int32_t *slots = malloc(SLOTS_SIZE)))

        // NOT REACHED

        slots[slot] = id;

        return slots;
}

What’s the difference between the first and the third? In the third, if malloc fails, it does not return NULL. It instead hands the program off to a recovery regime, which, by default, safely and immediately ends the program.

What’s the difference between caller-checks and callee-checks?

First, callee-checks is safer. You can’t screw it up. The worst you can do is write a program that will abruptly terminate. This is far better than the current worst-case scenario, in which manifestly common programmer errors allow Mark Dowd to upload malicious code into your program.

Second, callee-checks is cleaner. In the caller-checks case, not only does “setup” need to check, but so does “setup“‘s caller, and it’s caller’s caller, and it’s caller’s caller’s caller, all the way down to the place where your program inevitably gives up and terminates the program.

“But, Thomas”, you say, “not all programs do give up and abort. Some have policies for handling out-of-memory conditions”. And so they do. And in most cases, those policies are global, and can simply be substituted for the default behavior of exiting the program.

But I will grant you that in many cases, you have a genuinely useful recovery regime that is specific to one code-path —- say, an arena-style allocation regime for a particular user request —- and no global policy will help. So, I submit to you a fourth option, which I will not name, and which looks suspiciously like example (2):

#define hostile /**/

void *
_setup(unsigned hostile slot, unsigned hostile id) {
        u_int32_t *slots = NULL;

        if((slots = unsafe_malloc(SLOTS_SIZE)))
                slots[slot] = id;

        return slots;
}

Did you see the difference? It’s subtle. But it’s also easy to grep for and easy to check.

I am an advocate for checking malloc —- callee-checks style. It is simply harder to screw up, and, in the overwhelming majority of cases, which you can check for yourself by randomly sampling Google Code Search, it costs you nothing in terms of reliability of functionality. Stop caller-checking malloc.

To LeBlanc’s other point about C++ and constructors throwing exceptions, I refer him to Cargill, or back to our blog, noting that exceptions are themselves inherently dangerous. “When a language feature requires you to be that-language-feature-safe”, I believe I said, “you have a security problem”.

As for his specific example: you can’t blindly throw exceptions from a ctor. Even Meyers (MECv1, Item 9) catches that one. DON’T DATE ROBOTS!


Comments


The information, views, and opinions contained on this page are those of the author and do not necessarily reflect the views and opinions of SecurityFocus.






 

Privacy Statement
Copyright 2007, SecurityFocus