Casting a void pointer has always been wrong in C even though *everybody* does it. With malloc() there is at least one other subtle but possibly catastrophic problem. If you forget to include stdlib.h and you are casting all of the malloc return values the compiler will not give you a hint that the malloc prototype is missing. The return value will then default to integer. Now if you or somebody else tries to use your software on some platform where integers and pointers have different sizes or semantics you have a bug. If you're really unlucky the bug will even be exploitable.
tqbf
· 3 years ago
Great point. This is the one bug I ever spotted in qmail (but it's not exploitable there).
Don't you love that just missing a header file can create vulnerabilities?
daniel
· 3 years ago
I liked this and I agree except that you say that when malloc fails, exit inmediatly. I say this is a terrible way of doing things, what happens to the memory prevously allocated ? are you going to leave it there and assume it gets free'd ? This might not impact modern OS'ses but what about embeded devices etx ? I hope to hear your opinion on this. -daniel
Anonymous
· 3 years ago
Daniel, it's not a good idea to exit immediately in an embedded system. But exiting immediately from a normal desktop program where the malloc:ed memory is freed during exit of the program is no problem. If you have other resources allocated, you of course should free this during exit. Why not replace exit()? /Matt
dhelder
· 3 years ago
Re #1:
Once I had to debug memory corruption caused by something like this: s = malloc(10000) s[1025] = 'a';
malloc returned NULL but the assignment didn't segfault. What happened?
The problem was the page at 0 was protected, but the next page wasn't. s[1025] = 'a' was writing to somewhere on the heap. This was a pain to debug.
I prefer using a malloc() wrapper that asserts on the return value or just asserting on return value if there isn't one. Not checking the return value of a function is suspect to me.
Patrick O
· 2 years ago
Your comments about trusting malloc are spot on--we really should trust it to do what its supposed to. I've seen lots of programs (particularly embedded ones) where malloc lets you to configure a function pointer (or jump_buf) so you can get a callback on allocation failures... which then makes a great place for a debugger breakpoint.
Vineet Kumar
· 2 years ago
The sizeof example can be shortened further without loss of clarity. sizeof is an operator, not a function.
v = malloc(sizeof *v);
will do the trick. Less typing, easier to read.
The parentheses are only needed when you're naming a type, e.g. "malloc(sizeof (struct V))".
Joshua Haberman
· 2 years ago
I found #2 and #4 enlightening, and I think I'll start using those techniques in my code. I got in the habit of casting the return of malloc, and I *think* it's because I got compiler warnings when I didn't, but that was back in the day and I can't reproduce this warning with a modern gcc.
I can't agree with #1, at least when it comes to writing a library. It's just not the library-writer's place to make decisions like that for an application, and if the library-writer's answer to me was "just preload a malloc that does what you actually want" I'd be pissed. Preloading is a useful and clever hack when you need it, but it's no way to engineer the common case. What if one of your libraries statically linked libc into it? What if some libc implementation inlines malloc?
When I'm really investing a lot of effort into a library, or writing a library that is intended to be useful in space-constrained situations, I'll usually either avoid allocating memory internally or let the client pass me a pointer to their own malloc/free (with an additional user-defined void* so it can be reentrant, if desired). That gives the client maximal flexibility to define their own memory-allocation policy.
Vivek
· 2 years ago
I for one always thought that checking the value of malloc was stupid... If X's program is so badly written that the heap can overflow (several GB in todays date) without X being aware of that when writing the program, trying to fix the symptom by checking the value of malloc and exiting is a waste of X's time. Better that X advertise X's incompetency by letting the user see a SIGSEGV or Fatal error dialog box depending on the platform.
Thomas Ptacek
· 2 years ago
Joshua, I will have a hard time arguing with you on that point, but I will wag my finger and say that it is harder than it looks to write a library that can sanely and consistently handle memory exhaustion. I will win the argument that it is better to abort a program than to handle exhaustion less than perfectly. Never limp. Ever.
Luis Bruno
· 2 years ago
You might want to consider longjmp()'ing to a graceful_terminate() (or maybe use atexit()).
Thomas Ptacek
· 2 years ago
Things you can do with a preloaded malloc replacement, all good.
ryan
· 2 years ago
"...it’s 2006. Why are you wasting time with 'xmalloc'?"
It's 2007. Why are you wasting time with C?
kowsik
· 2 years ago
My past life (kernel networking code), I had very positive results by wrapping malloc/free into foo_malloc/foo_free, with one caveat: when you called foo_free, you had to pass in the size that you foo_malloc'd. This got set/validated by the foo_malloc/foo_free.
Especially when malloc/free were in separate modules (passing pointers around), this caught all kinds of bugs (arrays of arrays, etc) that assumed different things. Don't seem to have this problem anymore with Ruby. ;-)
"About number 2: Casting a void pointer has always been wrong in C even though *everybody* does it."
nope... unfortunately, you forget that on most systems malloc used to return a char *. (char *malloc(unsigned size);) the cast was a necessity. pre ansi-C89 void pointers were not even guaranteed on your compiler -- much less the behavior regarding their assignment.
when c++ eventually became its own forked language (still pre-ansi-c89) this behavior remained (though i believe c++ adopted the void * return value for malloc as well).
EpitacioLemos
· 10 months ago
Honestly, my opinion is that one may consider usign an exception handling library. If you want to follow old-school C programming techniques (which is a valid argument), I would suggest using a errno-like global variable to store the cause of the fault and then do check for errors all the way back to main(). Functions would return the so-called -1 on error, which would be checked when called (possibly with a ALL-CAPS satellite macro to perform the check more verbosely), but generally ignore the errno-like value (unless if it really needs to). Then, centralize all the user error messages in one function. This way, the resources can be unallocated easily, and you can reuse the code easily and in many scenarios.
All Comments
Casting a void pointer has always been wrong in C even though *everybody* does it. With malloc() there is at least one other subtle but possibly catastrophic problem. If you forget to include stdlib.h and you are casting all of the malloc return values the compiler will not give you a hint that the malloc prototype is missing. The return value will then default to integer. Now if you or somebody else tries to use your software on some platform where integers and pointers have different sizes or semantics you have a bug. If you're really unlucky the bug will even be exploitable.
Don't you love that just missing a header file can create vulnerabilities?
I hope to hear your opinion on this.
-daniel
/Matt
Once I had to debug memory corruption caused by something like this:
s = malloc(10000)
s[1025] = 'a';
malloc returned NULL but the assignment didn't segfault. What happened?
The problem was the page at 0 was protected, but the next page wasn't. s[1025] = 'a' was writing to somewhere on the heap. This was a pain to debug.
I prefer using a malloc() wrapper that asserts on the return value or just asserting on return value if there isn't one. Not checking the return value of a function is suspect to me.
v = malloc(sizeof *v);
will do the trick. Less typing, easier to read.
The parentheses are only needed when you're naming a type, e.g. "malloc(sizeof (struct V))".
I can't agree with #1, at least when it comes to writing a library. It's just not the library-writer's place to make decisions like that for an application, and if the library-writer's answer to me was "just preload a malloc that does what you actually want" I'd be pissed. Preloading is a useful and clever hack when you need it, but it's no way to engineer the common case. What if one of your libraries statically linked libc into it? What if some libc implementation inlines malloc?
When I'm really investing a lot of effort into a library, or writing a library that is intended to be useful in space-constrained situations, I'll usually either avoid allocating memory internally or let the client pass me a pointer to their own malloc/free (with an additional user-defined void* so it can be reentrant, if desired). That gives the client maximal flexibility to define their own memory-allocation policy.
If X's program is so badly written that the heap can overflow (several GB in todays date) without X being aware of that when writing the program, trying to fix the symptom by checking the value of malloc and exiting is a waste of X's time.
Better that X advertise X's incompetency by letting the user see a SIGSEGV or Fatal error dialog box depending on the platform.
'xmalloc'?"
It's 2007. Why are you wasting time with C?
Especially when malloc/free were in separate modules (passing pointers around), this caught all kinds of bugs (arrays of arrays, etc) that assumed different things. Don't seem to have this problem anymore with Ruby. ;-)
See: http://labs.musecurity.com/2007/07/23/writing-c...
nope... unfortunately, you forget that on most systems malloc used to return a char *. (char *malloc(unsigned size);) the cast was a necessity. pre ansi-C89 void pointers were not even guaranteed on your compiler -- much less the behavior regarding their assignment.
when c++ eventually became its own forked language (still pre-ansi-c89) this behavior remained (though i believe c++ adopted the void * return value for malloc as well).
If you want to follow old-school C programming techniques (which is a valid argument), I would suggest using a errno-like global variable to store the cause of the fault and then do check for errors all the way back to main(). Functions would return the so-called -1 on error, which would be checked when called (possibly with a ALL-CAPS satellite macro to perform the check more verbosely), but generally ignore the errno-like value (unless if it really needs to). Then, centralize all the user error messages in one function.
This way, the resources can be unallocated easily, and you can reuse the code easily and in many scenarios.