CERT C Secure Coding Standard: last call for reviewers [LWN.net]

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 14, 2008 20:17 UTC (Fri) by ajross (subscriber, #4563) [Link]

And this one steps right into a pet peeve of mine:

https://www.securecoding.cert.org/confluence/display/seccode/MEM32-C.+Detect+and+handle+critical+memory+allocation+errors

Yes, malloc() can return null. Dereferencing a pointer from malloc() can potentially crash your application in an OOM situation. So what do they suggest doing instead? Nothing! They just give you some example code with (I'm not kidding) "/* Handle Error */" marked! Sigh... As if you could just paste in a little code to fix this problem...

OOM situations on modern (multiprocess, virtual memory) operating systems are not recoverable errors at the level of a single application. In most cases, the proper behavior really is just to crash, terminate, and let a human being clean up the mess. There is nothing you can do from inside the application to automatically diagnose and treat an out of memory situation that was probably caused in some other process, and even if there were, the code you would need to call to do that requires more memory to do so.

The core point is this: if your application has security requirements that rely on it not failing in the presence of memory exhaustion (and, yes, some apps do), then it has design flaws at a far more fundamental level that merely the malloc() checking. Applications that truly live in that space generally have more elaborate rules, like "Don't use dynamic memory allocation at all". Putting this item in as part of a "tips and tricks" list from CERT is an insult to the intelligence of the people who actually think about these problems.

No one will be helped by this rule. All that it will produce is a bunch of boilerplate error "handling" code that doesn't work.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 14, 2008 21:24 UTC (Fri) by rahvin (subscriber, #16953) [Link]

I hope you are also addressing this comment to CERT as that is exactly what they are asking
for is it not?

recovery from NULL==malloc(size)

Posted Mar 14, 2008 21:38 UTC (Fri) by jreiser (subscriber, #11027) [Link]

OOM situations on modern (multiprocess, virtual memory) operating systems are not recoverable errors at the level of a single application.

Sometimes they can be, but it requires planning ahead, perhaps from the very beginning of the application which wishes to recover. One strategy is to malloc a safety buffer (say, 5MB), scribble on it, not use it after that, then free() it upon running out. In some situations this buys enough for orderly shutdown, switching modes, or postponement of the application.

... more elaborate rules, like "Don't use dynamic memory allocation at all."

By itself, such a rule is as simple as possible; but implementing it might be cumbersome and elaborate. Or, it might be a true inspiration: make a pre-pass which computes a close upper bound, perform all the malloc() at one time [and do anything else needed to "fully" consume the memory resources], then run the main application which sub-allocates space from the reservations.

recovery from NULL==malloc(size)

Posted Mar 15, 2008 0:42 UTC (Sat) by aleXXX (subscriber, #2742) [Link]

> > ... more elaborate rules, like "Don't use dynamic memory allocation
> > at all." 
> By itself, such a rule is as simple as possible; but implementing it 
> might be cumbersome and elaborate.

Yes, it's doable but requires quite some effort, which makes sense e.g. 
for embedded systems, where it helps to have the memory requirements 
checked at link time and where you don't want to waste time in malloc().

But as soon as you use some libraries, you probably use dynamic memory. 
STL uses it a lot, so does Qt. C libraries like libxml (it needs to store 
the results somewhere) and gtk probably too.

Alex

recovery from NULL==malloc(size)

Posted Mar 15, 2008 17:43 UTC (Sat) by elanthis (subscriber, #6227) [Link]

STL does not necessarily use dynamic memory allocation.  It makes use of a pattern called
allocators, which make it very possible to use static memory allocation for all objects,
including the internal objects used by STL containers.  This is used in embedded systems quite
a bit.

There are also variations of the STL and various other libraries that are designed for
embedded systems that avoid dynamic memory allocation.  You should see some of the development
frameworks used for making Gameboy, DS, and PSP games.  Unlike the Java games you see on
phones and such, those games push their respective hardware to the limit and need heavy
control over all memory allocations to get there.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 14, 2008 23:45 UTC (Fri) by nix (subscriber, #2304) [Link]

Actually I was just saved a pile of debugging by this rule. I religiously 
check for allocation failures, and actually have a bunch of macros to 
assist: they provide a crude C exception-unwinding/deallocate-on-failure 
facility, too, but most importantly they enable me to *distinguish* 
between failure sites.

In this specific case the allocation failure was *right* at the place 
where a huge leak was happening (as is likely when the leak is huge: a bit 
of dmallocing and all the other leaks were fixed, too). Without error 
trapping, all I'd have got would have been a core dump, and as the failure 
happened on a live site which won't let us see core dumps because they 
might well contain confidential data, I'd have been stuffed.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 15, 2008 5:15 UTC (Sat) by PO8 (guest, #41661) [Link]

I think the point of this example is that you *can't* count on your program to crash at the
point of the failed malloc().  Having "/* Handle Error */" be "abort()" is OK---assuming that
strcpy() or whatever will magically abort() for you is not.  If the program *doesn't* crash or
handle the error at this point, you have all kinds of potential security vulnerabilities...

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 16, 2008 19:16 UTC (Sun) by wahern (subscriber, #37304) [Link]

I check all malloc() calls, and gracefully handle the condition. I don't find this difficult,
and it pains me to hear people say that it's "impossible" to handle OOM conditions. Unless
you're writing a shell utility, any medium sized or large application does a myriad of
operations, many or most of which can be postponed, or which can themselves fail individually
without forcing the rest of the application to exit.

You also don't consider process resource limits. Because there are so many lazy developers
*cough* who don't handle memory allocation properly, its prudent for administrators to limit
the amount of memory processes can use, not the least of which is because one hungry process
(broken or not) can take down other processes written by said lazy developers. This applies
whether or not you disable a kernel's lazy allocation "feature".

This goes for server applications as well as desktop applications. If I try to load an image
in a photo editor, I don't want to lose all of my work because I loaded one too many images.
Likewise, if I have a network application servicing multiple connections, I don't want to take
down all the connections just because one connection attempted an operation which innocently
hit some arbitrary memory limit; rather, I want to return failure, or, at the most, disconnect
that client.

Shameful.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 16, 2008 20:19 UTC (Sun) by nix (subscriber, #2304) [Link]

`Gracefully' is an interesting question. In C, without the option of 
throwing exceptions and letting the unwinder handle deallocation, it's 
often very annoying to clean up after malloc()s. obstacks make it easier 
but make so many other things harder that it's often not worth it; 
Apache-style mempools are good as well... but often I arrange to partition 
the system into processes which leave notes in some shared communication 
medium, then simply die, such that some controlling process can detect 
when a process has OOMed or suffered some other transient failure, and can 
clean up and restart them appropriately. I'm not sure if 'just die' counts 
as 'graceful', though. (All this is probably easiest if the processes 
spend most of their time talking to transactioned things like databases, 
because the 'clean up' phase is trivial.)

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 16, 2008 21:53 UTC (Sun) by wahern (subscriber, #37304) [Link]

There are certainly many different ways to handle this. Like many things, much of it comes
down to choice of data structures, code flow, etc. You learn to balance these things, and to
program in a way which minimizes the intrusion of error control in the logic.

Of course, many things in C are tedious--compared to so many scripting and so-called "managed"
languages--so "graceful" is relative. But those people who think its too tedious to handle it
shouldn't be using C. There are many other wonderful languages and application environments to
use, and I use them often myself. If someone thinks bailing on malloc() should be S.O.P., then
maybe they ought to use a Web Application paradigm, where operations--and failures--are
inherently isolated by whatever means the web server uses. There's nothing wrong with
leveraging those tools, and those authors' investments. That's what Free Software is all
about. Unfortunately, too often people want to get down and dirty and write "efficient" code
in C, but then complain when they have to manage the chores, too.

Of course, most of this goes for other languages, like Java, C#, Perl, etc when developing
applications of equivalent complexity; but I can't help it if this lackadaisical attitude has
already gripped those spheres. Exceptions and GC only go so far when a programmer is more
interested in doing the fun stuff, and couldn't care less about the tedious chores. People use
software to get things done; they want software writers' to make the _problems_ disappear
(i.e., to be handled automatically, not dropped on the floor). This is a contract which
shouldn't be broken lightly.

In my experience, though, handling malloc() is no different than handling a malformed HTTP
request, or any other exceptional condition that occurs when an application handles external
resources and data. More often than not, where one finds a function that "gracefully" handles
malloc() failure it also handles other errors. malloc() isn't any different than fopen()
failing, or read() failing, etc. No reason to treat it differently; there's no better way to
"fix" a read()--as alluded to by the OP--than malloc(). If a client disconnects abruptly, I
can't introspect his process stack to "fix" the problem. I just clean up and move along. This
is simply the general pattern of error handling, and it always requires releasing any
intermediate resources. (I've written allocator pools and whatnot, but after a few years I
realized it doesn't actually help that much in terms of effort; if anything it's better just
as a performance boost for small string allocations, or for finer-grained memory limits and
accounting. (See libarena)).

Granted, there are some failures that can't really be handled well, like a stack overrun
(though I've seen ingenious code which goes pretty far). But malloc() returns NULL for a
reason, and its not because the sanest thing to do is merely exit. Its no excuse, however, to
say that, because C is helpless in some respects to memory management, its futile to manage
all memory management.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 16, 2008 22:43 UTC (Sun) by nix (subscriber, #2304) [Link]

There is a reason why malloc() should be treated differently from, say, 
fopen() failing. A failing fopen() is unlikely to be transient and 
unlikely to be something that can be fixed without human intervention, nor 
is it likely to make other unrelated things fail (other fopen()s will 
probably work, as will, say, fwrite()). malloc() has neither of these 
properties.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 17, 2008 1:31 UTC (Mon) by wahern (subscriber, #37304) [Link]

What do you mean by "unlikely"? If you mean to say that the scenarios where one fails and the
other doesn't, then of course there are differences. But, I fail to see how that means they
should be treated different when they fail.

Noting this distinction is part of the original faulty logic, which is that a prerequisite
condition of managing malloc() failure is that the why's and how's need be somehow discernible
by the application, otherwise its a futile endeavour. Ostensibly, this is so the application
can "fix" them, or alter its behavior according to the specific reasons. I don't... it's hard
to discern the motivations or intentions in that line of argument. I can't see how the
requirement of "keep running" is conditioned on whether MySQL has a memory leak, because a
user just loaded a 200MB tiff image in Firefox, or because the /tmp partition filled up.

A robust application, by definition, should handle any _potentially_ transient failure
regardless of the underlying cause, generally speaking (there are myriad exceptions, of
course). A robust application can and should strive to remain internally consistent and
stable. Arguing that handling malloc() different from fopen() because one condition holds true
for malloc() 90% of the time, but only 10% of the time for fopen(), makes absolutely no sense
in a general context. And general context is what we're talking about: the proper practice for
handling malloc() failure.

What might be 10% or 90% generally might be 1% or 100% for any particular user or environment.
Different users experiencing wildly different behavior from software is one of the stains on
the industry, and its specifically because of these dependencies on unwarranted assumptions
and distinctions. Bugs will happen, and reasonable people can argue about what's easy or
difficult, but its ludicrous when the intentional practice is defended.

Once an application has hit a steady state (if it's that sort of application, like a desktop
application, server daemon, or even a really long lived data processor, etc), malloc() failure
should never cause it to exit. Even if it has a watchdog to restart it, these are the types of
applications which juggle multiple tasks, and which the unnecessary loss of state creates
gigantic headaches; they demand internal isolation of all reasonable errors. And arguing that
handling a malloc() failure is unreasonable doesn't hold water, particularly after the
application has already enough committed resources to subsist. This almost never requires
keeping "emergency" memory buffers or any other tricks; I've never had to resort to such
things. It's the proverbial case of throwing the baby out with the bath water.

Imagine, for instance (with the infamous exception of glib) libraries exiting like this? If
OpenSSL crashed your application because it couldn't initialize an internal resource? OpenSSL
is an incredibly complex piece of software; probably too complex, with lots of cruft. But they
manage to do this "impossible" task, and don't resort to making distinctions between this
resource or that resource. It propagates the condition to the appropriate level, which is the
portion of code which decides whether its remainder can proceed with the failure of the
_logical_ task, and (similar to the issue above), not based on why, specifically, it failed.
And because of the miracle of componentization and abstraction, the problem of handling
malloc() failure becomes identical to handling any of the other potential failure conditions.
And unless you can make a case in a particular instance for bailing on all such errors, why
bail on only some of them?

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 20, 2008 15:13 UTC (Thu) by paulj (subscriber, #341) [Link]

The simplest and most reliable error-strategy often is to restart, surely? Optimising restart
has benefits for more than just error-handling too.

The obvious example would be malloc() failing because the application itself is leaking
memory. No amount of retrying malloc() will help there. Restarting will however at least
mitigate the problem, allowing the user to continue to use the system, until the software can
be fixed to address the leak.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 18, 2008 14:30 UTC (Tue) by jond (subscriber, #37669) [Link]

Interesting that they use the form "ptr == NULL" in their comparisons, rather than
constant-first comparison to avoid accidental assignment typos. Wasn't that the root cause of
at least one severe X.org security bug in the last few years?

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 14, 2008 22:45 UTC (Fri) by endecotp (guest, #36428) [Link]

> Strict adherence to this rule, for example:
> https://www.securecoding.cert.org/confluence/display/secc...
> ...would seem to disallow any use of dlopen()/dlsym() 

The linked-to rule says: "Do not convert a function pointer to an incompatible type".  When
you call dlsym you get a void* which you cast to the function pointer type; you're not
"converting a function pointer to" anything and so not breaking their rule, as far as I can
see.

Their example is:

static void my_function(int a)
..snip..
  int (*new_function)(int a) = my_function;
  x = (*new_function)(10);

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 15, 2008 0:10 UTC (Sat) by nix (subscriber, #2304) [Link]

Note that casting void to a function pointer *is* an extension to the C 
Standard. I can think of several platforms on which it might not work 
without special handling (generally those like HPPA or IA64 on which 
function pointers are elaborate descriptors of some kind: of course that 
special handling has to be provided, because a Unix these days has to have 
dlsym()...)

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 15, 2008 3:31 UTC (Sat) by ajross (subscriber, #4563) [Link]

No, it works fine everywhere in the modern world.  The reason for that restriction in the C
standard is that historically there have existed "Harvard Architecture" machines where the
code and data areas are physically separate regions indexed with overlapping pointer values.
The only one I can think of off the top of my head that still exists in common use is the 8051
microcontroller.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 15, 2008 11:32 UTC (Sat) by Los__D (subscriber, #15263) [Link]

In fact "Harvard Architecture" CPU's doesn't (necessarily) reference different areas, just
different busses for code and data.

Thus, any modern CPU, more or less uses Harvard Architecture because of the split code/data
cache design.

- Which is, of course, completely irrelevant to your point, carry on. :)

DSPs use Harvard architecture

Posted Mar 16, 2008 6:29 UTC (Sun) by JoeBuck (subscriber, #2330) [Link]

... and if you have a cell phone you run one. Separate program and data space, and sometimes the busses are different widths.

CERT C Secure Coding Standard: last call for reviewers

Posted Mar 17, 2008 22:00 UTC (Mon) by im14u2c (subscriber, #5246) [Link]

It only really matters if you try to compare a (void *) that points to a function against a (void *) that points to data, or if function pointers have a much different cost to carry around (e.g. one pointer type is larger than the other). The latter could happen back in the old 16-bit x86 days, depending on your memory model. For example, "near" code (16 bit offset-only pointer) vs. "far" data (32-bit segment:offset pointer) or vice versa.