Before the lifetime of the last pointer that stores the return value of a call to a standard memory allocation function has ended, it must be matched by a call to free()
with that pointer value.
Noncompliant Code Example
In this noncompliant example, the object allocated by the call to malloc()
is not freed before the end of the lifetime of the last pointer text_buffer
referring to the object:
#include <stdlib.h> enum { BUFFER_SIZE = 32 }; int f(void) { char *text_buffer = (char *)malloc(BUFFER_SIZE); if (text_buffer == NULL) { return -1; } return 0; }
Compliant Solution
In this compliant solution, the pointer is deallocated with a call to free()
:
#include <stdlib.h> enum { BUFFER_SIZE = 32 }; int f(void) { char *text_buffer = (char *)malloc(BUFFER_SIZE); if (text_buffer == NULL) { return -1; } free(text_buffer); return 0; }
Exceptions
MEM31-C-EX1: Allocated memory does not need to be freed if it is assigned to a pointer whose lifetime includes program termination. The following code example illustrates a pointer that stores the return value from malloc()
in a static
variable:
#include <stdlib.h> enum { BUFFER_SIZE = 32 }; int f(void) { static char *text_buffer = NULL; if (text_buffer == NULL) { text_buffer = (char *)malloc(BUFFER_SIZE); if (text_buffer == NULL) { return -1; } } return 0; }
Risk Assessment
Failing to free memory can result in the exhaustion of system memory resources, which can lead to a denial-of-service attack.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
MEM31-C | Medium | Probable | Medium | P8 | L2 |
Automated Detection
Tool | Version | Checker | Description |
---|---|---|---|
Astrée | 24.04 | Supported, but no explicit checker | |
Axivion Bauhaus Suite | 7.2.0 | CertC-MEM31 | Can detect dynamically allocated resources that are not freed |
CodeSonar | 8.1p0 | ALLOC.LEAK | Leak |
Compass/ROSE | |||
2017.07 | RESOURCE_LEAK ALLOC_FREE_MISMATCH | Finds resource leaks from variables that go out of scope while owning a resource | |
Cppcheck | 2.15 | memleak leakReturnValNotUsed leakUnsafeArgAlloc memleakOnRealloc | Doesn't use return value of memory allocation function |
Cppcheck Premium | 24.9.0 | memleak leakReturnValNotUsed leakUnsafeArgAlloc memleakOnRealloc | Doesn't use return value of memory allocation function |
Helix QAC | 2024.2 | DF2706, DF2707, DF2708 C++3337, C++3338 | |
Klocwork | 2024.2 | CL.FFM.ASSIGN CL.FFM.COPY CL.SHALLOW.ASSIGN CL.SHALLOW.COPY FMM.MIGHT FMM.MUST | |
LDRA tool suite | 9.7.1 | 50 D | Partially implemented |
Parasoft C/C++test | 2023.1 | CERT_C-MEM31-a | Ensure resources are freed |
Parasoft Insure++ | Runtime analysis | ||
PC-lint Plus | 1.4 | 429 | Fully supported |
Polyspace Bug Finder | R2024a | CERT C: Rule MEM31-C | Checks for memory leak (rule fully covered) |
PVS-Studio | 7.33 | V773 | |
SonarQube C/C++ Plugin | 3.11 | S3584 | |
Splint | 3.1.1 | ||
TrustInSoft Analyzer | 1.38 | malloc | Exhaustively verified. |
Related Vulnerabilities
Search for vulnerabilities resulting from the violation of this rule on the CERT website.
Related Guidelines
Key here (explains table format and definitions)
Taxonomy | Taxonomy item | Relationship |
---|---|---|
ISO/IEC TR 24772:2013 | Memory Leak [XYL] | Prior to 2018-01-12: CERT: Unspecified Relationship |
ISO/IEC TS 17961 | Failing to close files or free dynamic memory when they are no longer needed [fileclose] | Prior to 2018-01-12: CERT: Unspecified Relationship |
CWE 2.11 | CWE-401, Improper Release of Memory Before Removing Last Reference ("Memory Leak") | 2017-07-05: CERT: Exact |
CWE 2.11 | CWE-404 | 2017-07-06: CERT: Rule subset of CWE |
CWE 2.11 | CWE-459 | 2017-07-06: CERT: Rule subset of CWE |
CWE 2.11 | CWE-771 | 2017-07-06: CERT: Rule subset of CWE |
CWE 2.11 | CWE-772 | 2017-07-06: CERT: Rule subset of CWE |
CERT-CWE Mapping Notes
Key here for mapping notes
CWE-404/CWE-459/CWE-771/CWE-772 and FIO42-C/MEM31-C
Intersection( FIO42-C, MEM31-C) = Ø
CWE-404 = CWE-459 = CWE-771 = CWE-772
CWE-404 = Union( FIO42-C, MEM31-C list) where list =
- Failure to free resources besides files or memory chunks, such as mutexes)
Bibliography
[ISO/IEC 9899:2011] | Subclause 7.22.3, "Memory Management Functions" |
56 Comments
Mark Dowd
Maybe there needs to special mention of the UNIX realloc() gotcha here - if the size passed to realloc() is 0, realloc() frees the memory you pass, rather than attempting to reallocating it. This can lead to double-free problems..
John McDonald
The C standard doesn't say anything about what happens with a size argument of 0, but I checked glibc and openbsd source real quick and they appear to return a valid pointer to a zero-sized object.. e.g. the return of a malloc(0);
I mention this because if there was a system where realloc() free'ed the object and then returned NULL, it would make the secure idiom for realloc() exploitable. Here's that idiom, snagged from an openbsd man page:
You can see that if nsize was 0, and realloc() free'ed the memory and then returned NULL, it would be a double-free of p. I would guess that no systems actually do this, but it might be worth researching. As it stands, the behavior with an nsize of 0 is pretty interesting and counter-intuitive, and I can see people running into trouble because of it.
Robert Seacord
OK, I've updated MEM36-C. to talk about reallocating zero bytes.
Jonathan Leffler
Solaris 10 (UltraSPARC) specifically returns NULL when you realloc 0 bytes.
This code yields "s was not null" and "p is null".
Stephen Friedl
This again points to the benefit of setting pointers to NULL after free(); this doesn't fix the logic error that calls free() twice, but it at least gets rid of the gross security issue of freeing a random pointer.
Douglas A. Gwyn
I prefer if(x!=NULL)free(x) ; since it doesn't depend on the special case for free(NULL) which some libraries might get wrong. (Before C89, almost all of them croaked.)
Jonathan Leffler
The document assumes C99 - even if it only assumed C89, free(0) is defined as safe. It is only pre-C89 implementations that might have the problem. Presumably, those who work on such implementations are aware of the problems, and for the large majority who do not work on such archaic systems, there is no issue.
Dhruv Mohindra
I recall a discussion about the comments that say "handle error". Perhaps the CS would be better if it exits or returns (with an error code) after freeing the memory on error_condition = 1 rather than removing
free()
altogether from that part of code.David Svoboda
I think you are referring to code that does the following:
This code clearly frees x just once, and so complies with the rule, but this code has the following drawbacks:
x
is duplicated inside & outside the if statement. The redundancy leads to code bloat and potential for more errors.I suspect these reasons are why this code sample wasn't included as a 2nd CS.
Martin Sebor
I agree that the rule would be more "believable" if the error handling was less handwavy. Maybe like so:
Dhruv Mohindra
The above code by Martin appears to address the return problem/possible memory leak I'd imagined. +1
Martin Sebor
I've updated the code examples.
David Svoboda
MSVC error C6308 addresses an improper use of realloc(). which rule corresponds with this diagnostic? I assume its MEM31-C, but that's not obvious. Whichever rule should handle C6308 needs a NCCE & CS pair to illustrate the problem.
Martin Sebor
I wonder if MEM08-C. Use realloc() only to resize dynamically allocated arrays could be tweaked to cover the case you mention. As it stands, the problem MEM08-C tries to avoid doesn't seem like one that's specific to
realloc()
but rather one of converting a pointer to one object to a pointer of an unrelated type. That can just as easily happen withmalloc()
or in any other situation involving a cast of a pointer to another.David Svoboda
We may want to add an exception for dynamically-loaded libraries. Most programs allocate memory to hold dynamically-loaded code (eg the LoadLibrary() function in Windows), and do nothing to free them (expecting that they will persist through the lifetime of the program.)
Robert Seacord (Manager)
This rule is going to get trashed and replaced by something that looks like the memory portions of this TS 17961 rule:
5.18 Failing to close files or free dynamic memory when they are no longer needed [fileclose]
John Whited
Even when replaced, the Risk Assessment needs attention.
Severity on this page is "Medium". But summary on the "Memory Management" page shows Severity for this rule to be "High". The two should align.
Jonathan Headland
In the example code above, there's a line which reads:
char
*text_buffer = (
char
*)
malloc
(BUFFER_SIZE);
I don't think that cast to
char *
should be there. It's redundant, since C implicitly converts a pointer-to-void to any pointer type, and the cast could hide a bug if the call tomalloc()
were to be replaced by a call to a function which didn't returnvoid *
.Aaron Ballman
While not strictly needed, it's commonly viewed as a best practice to do the cast. See MEM02-C. Immediately cast the result of a memory allocation function call into a pointer to the allocated type for more information.
Jonathan Headland
Agreed. Thanks.
Joseph C. Sible
To quote Raymond Chen, "There’s no point sweeping the floors when the building is being demolished."
I think we should have another exception in line with that. Freeing memory immediately before the process exits is just a waste of time. For example, if your strategy to handle a failing
realloc()
is to callexit()
orabort()
, then you don't need to worry about keeping the old pointer around.Aaron Ballman
I support this and would point out that EX1 is providing basically this exact exception already (it can likely be reworded to make this more clear, however). It may need some weasel words about verifying that the host OS cleans up memory on program termination to cover embedded systems which may not behave the same way as desktop systems.
Joseph C. Sible
It definitely looks like that was the intent of it, and it almost does that, but the restriction that the pointer must have static storage duration really limits its applicability.
David Svoboda
This point deserves more thought. Given the pushback we've gotten on other rules, I'm a little surprised that we haven't addressed this argument earlier. Possibly because while calling abort() or exit() without freeing memory can still produce safe code, it is sloppy practice, and writing a rigorous exception that permits this safely looks tricky.
For that reason, I would rather not modify EX1, but rather add a new exception about precisely when this is permissible.
There are probably other corner cases I'm missing. Comments?
Joseph C. Sible
exit()
. Would this exception really cause new problems there?abort()
.main()
.Jonathan Headland
main()
That's only safe if
main()
isn't called recursively, surely?Joseph C. Sible
Yes, that's a good point.
Aaron Ballman
The situation I'm talking about is where you're about to return from
main()
and you elect not to free memory. This situation is exactly identical to callingexit()
(because that's what returning frommain()
is defined to do). FWIW, this is a very common practice on desktop systems – I just used this pattern in yet another project and reduced my runtime by a significant amount simply by letting the OS reclaim memory rather than walking lists and freeing manually before exiting.Not running a nontrivial destructor is a different kind of issue than this one. I would definitely allow this exception for C++ as it relates only to allocated memory.
I would not be prescriptive like this. I would modify the exception:
Allocated memory does not need to be freed if it is assigned to a pointer <del>with static storage duration</del> whose lifetime is the entire execution of a program. The following code example illustrates a pointer that stores the return value from
malloc()
in astatic
variable:and then add another example where the memory is not in a static variable, such as:
David Svoboda
First, I'm willing to modify the exception if we decide that what we want is close enough to it. But let's figure that out first before deciding if it should be separate or part of the current exception.
I'll agree that whatever permissions we put on calling exit() or _exit() can also apply to returning from main().
If you allow memory to be leaked via exit() (or main()), then you have just defanged this rule...when would code ever violate it? FTM your code example is very similar to the noncompliant code example.
Joseph C. Sible
It's definitely kind of a fuzzy boundary. Here's a few ideas that popped into my head:
free()
is often only made reusable by a futuremalloc()
and not given back to the OS until the program exits anyway)Aaron Ballman
I don't think this defangs the rule as the rule is already only intended to cover the cases where the memory is no longer needed (after the program terminates, the memory cannot be needed by the program). The issue is that we need to define "no longer needed" sufficiently.
David Svoboda
Precisely. Right now "no longer needed" applies to all dynamic memory, except those covered by EX1. Which is a bit restrictive, but sufficiently precise to justify this being a rule.
One option would be the "valgrind strategy". If dynamic memory is still allocated once it is no longer referenceable, before the program exits, that is a well-defined violation of this rule. Hard to manually enforce, but valgrind does it automatically. And that lets you exit the program with memory still allocated (and not covered by the exception.)
Aaron Ballman
Yeah, I see what you mean about the problem now – there's no way to tell the difference between an unwanted pointer you've forgotten to free and an unwanted pointer you only want to free at program termination time, so this requires understanding programmer intent. Blech.
My concern with the Valgrind algorithm approach is that this rule is also used in C++ and the story gets more complicated there. Say, for instance, that I have an automatic variable of type
std::vector<void *>
where the pointers are allocated and this automatic goes away within the scope ofmain()
; those pointers are leaked per the Valgrind algorithm, but the user has no way to prevent that situation from occurring, either. e.g.,While this is obviously a contrived example, this sort of scenario can easily crop up due to destructors (even within functions called within
main()
).The primary reason this is a security rule is because we want users to not run out of resources while the program is executing. I wonder if we can reword the rule to be less about which pointers are required to be freed and which ones aren't and more about resource exhaustion, somehow. Then again, that may still not be enough to avoid the programmer intent issue.
Regardless, I think this is a real deficiency with the rule that needs addressing somehow. This approach is not an uncommon one (esp in C++, but also in C) and has no negative security implications on modern desktop OSes (that I've been able to find evidence of).
David Svoboda
Aaron:
I don't follow your example. A vector of raw pointers as you demonstrate violates RAII, and you could easily 'fix' this code example by iterating over the vector freeing its pointers before it goes out of scope.
I will also disagree that this is not a modern problem. Are you really saying that exhausting memory has no security implications on modern systems??? Certainly on an otherwise secure system, exhausting memory will do nothing more than slow down the system. But when memory gets close to exhaustion, then malloc() starts returning NULL for all programs on the system, and, sadly, few programs handle this gracefully. So you get lots of seemingly random breakage in unexpected places. Unfortunately, this is not a theoretical conjecture
While we can't discern programmer intent, a programmer who wishes to not deallocate a pointer has several techniques for indicating this, the easiest is to make the pointer static (or make it part of a static data structure). Which means EX1 applies to it.
Aaron Ballman
Your assumption that this code is broken is a bit strange. It's a contrived example, but it comes up in real code and it's not a bug that needs to be fixed. The whole point to the concern is that iterating over the list to free the pointers before it goes out of scope is wasted time that has measurable performance impacts when the program is about to exit anyway, and failing to free those pointers has no negative security implications.
I said the only security implication with this is with resource exhaustion, so I'm not certain where you got this idea from.
And I'm saying that EX1 is insufficient because it restricts it to only handling the easy case while ignoring real world coding practices that should not be considered insecure.
FWIW, this comes up for me when I'm using an arena allocator instead of directly using the system allocator. When the program terminates, I can spend hours (literally) walking the list of allocations and freeing them with the system allocator, or I can just say "the arena is going away because the program is going away, skip those deallocations" and the program terminates almost instantaneously. I could not do that and comply with this rule as it is written because the arena-allocated pointers are not static.
David Svoboda
It looks like we agree on what your code example does, I just interpreted it differently than you. We both agree that it has no security problems on modern hardware, and it currently violates the rule. But it would be nice if we can add an exception to cover your code example.
There are several ways to tweak your code to be covered by EX1 without making it slower. Making either the vector static would be the easiest way. You could also make a static pointer variable and assign it to the vector, tho that's more work.
Your code example would also be covered by my 'valgrind strategy' exception, because the allocated chunk is always reference-able.
Aaron Ballman
Awesome, we've got agreement!
Making the vector static would not meet the requirements of EX1 because the pointers themselves do not have static storage duration just because they're stored within a list that has static storage duration. Assigning into a static pointer variable that then gets overwritten doesn't seem to really match the intention of EX1 either. For instance, we don't want to allow:
I don't think it would. The vector destructor is run before
main()
terminates, so the pointers it used to store are unreachable at the point the program terminates. Also, I think we don't want to split hairs with code like my original vector example above and code like below (where there is no functional difference in the program behavior):Joseph C. Sible
I think that's a bit too narrow of a reading of it. Consider this code:
Should EX1 really only apply to the first malloc, and not to any of the inner mallocs? Instead of only applying to things directly stored in static pointers, shouldn't it apply to anything reachable through static pointers?
David Svoboda
Joe, Aaron is right. I too had interpreted EX1 to include pointers that are owned by a static pointer, eg you had a static array of pointers to allocated chunks. Leaving that whole thing un-freed would be OK. Lots of GUIs do this, by making a static tree of GUI widgets & similar data and leaving it alive until termination. But the current reading of EX1 only applies to the top-level pointer.
Aaron is also right that the vector dtor would complete before main completes. For C++ we would have to define 'program termination' in this rule as after the last statement of main but before the first destructor of the scope executes. (For C we would also have to define program termination as preceding atexit() handlers).
I do suspect that burying your destructors in a 'func' function that is the last thing called by main starts us down a slippery slope. So I'm against that idea for now.
Aaron Ballman
FWIW, this has come up yet again: https://twitter.com/shafikyaghmour/status/1458965190020534276
It would be nice if the exception to this rule could be made more clear, because (as Shafik later pointed out), education in this area is a challenge.
David Svoboda
Coming back to this rule, I realized something about the introduction. As written, the introductory text specifically does not apply to allocated chunks that are still reference-able at program termination. (Technically this means we could dispense with EX1, such the pointers it covers didn't violate the rule anyway.)
This nicely sidesteps our entire conversation about pointers that are reference able upon termination (and precisely what constitutes termination). So most of the example programs we've discussed (including Shafik's example on Twitter).
This does introduce a security hole...a program can easily assign an allocated chunk to a pointer and (unintentionally) keep the pointer alive until termination. Which is in-distinguishible from Shafik's example). That is simply outside the scope of this rule.
While not strictly necessary, I'll add an explicit exception specifically about this shortly. UPDATE: I modified EX-1, which was easier than adding a new exception.
Joseph C. Sible
If a pointer to memory is stored in a variable scoped to the
main
function, does that count as being reachable at program termination? For example, is this okay?David Svoboda
Joseph C. Sible This code is "ok" in that it does not violate this rule. EX1 makes this more explicit, but the code would still comply with the rule if we took out EX1. The normative text, in the introduction, does not include this code, because the pointer's lifetime matches the lifetime of the program.
Joseph C. Sible
But is the end of main really the end of the lifetime of the program? Consider this GNU C program:
Or if you say that's an extension so it doesn't count, then consider C++, where code can run in static objects' destructors even after main returned. Would those cases still be okay for main to do that?
David Svoboda
There may be details of the __destructor__ attribute that elude me, and are out of scope, since it is not standard C. But I would suggest that your code does not violate this rule, and is in fact perfectly good code if your memory is still accessible once main() goes out of scope.
For now, I've used the phrase: "whose lifetime includes program termination". Suggestions for improved wording are welcome (including "when main() goes out of scope").
Aaron Ballman
Hmm, I guess I view the termination of the program as ending the lifetime of all pointers. (Termination as in "in a hosted environment, after the OS cleans up the no-longer-running-process".)
David Svoboda
There is clearly some wiggle room as to what constitutes "program termination". And there is a point in any C or C++ program then the program has nothing to do but clean up. (eg destructors or similar activities). And I'm losing interest in the "what is termination" debate for the purpose of this rule.
Aaron Ballman
I read EX1 as only applying to the first
malloc()
today, but agree that isn't a helpful exception. I think we will want to go with something like pointer reachability, but I still don't see why it has to be reachable through a static pointer specifically. In C, consider:If the arena uses
malloc()
internally, anddo_work()
does allocations through the arena, there's no need to free the entire arena in this case, despite it not being a static variable. While the user could always writestatic
there, I'm not certain I agree it should be a requirement to do so.Joseph C. Sible
As I look through other rules, I think we need another exception: strings that have been passed to
putenv
. From POSIX:David Svoboda
What is this 'putenv'? That be one of them new-fangled POSIX functions? It aint no C standard function, podner.
More seriously, what constraints does putenv() impose on its argument? Does it copy the argument contents, or does it require the argument's lifetime to last the life of the program (or until the env var is overwritten).
There is certainly some guideline you could write about safe use of putenv, but it belongs in the POSIX section.
Joseph C. Sible
We already wrote about
putenv
in the POSIX section: POS34-C. Do not call putenv() with a pointer to an automatic variable as the argumentThe problem is that using it safely means that you'll break this rule.
David Svoboda
The SEI CERT C Coding Standard addresses specific coding flaws that arise from writing standard-compliant C code. This means that your noncompliant code examples must not rely on POSIX or other OS-specific features, like putenv(). It is OK if your compliant code uses such features, but then you must mark the compliant solution as specific to POSIX.
We created the POSIX section for POSIX-specific coding problems. There is a similar section for Windows.
We do expect that each platform will introduce complications and new exceptions to the CERT C rules and recommendations, but such exceptions are out-of-scope to the rules & recs themselves.
Joseph C. Sible
We use noncompliant POSIX-specific code outside the POSIX section all over the place, e.g.:
Why is this case not okay if they are?
David Svoboda
Sorry, it is a fine point and I didn't make it fine enough. You are correct in that we do have platform-specific NCCEs. But all of those rules have platform-neutral NCCEs, which prove the point that you can violate the rule without relying on nonstandard functions.
While you can write unsafe code using POSIX's putenv() function, I'm not sure you can write unsafe code in the same way using strictly ISO-conforming functions.
Finally, insecure code relating to putenv() is already covered by POS34-C.
Joseph C. Sible
"all of those rules have platform-neutral NCCEs" Of my three links, only the first one does.
"Finally, insecure code relating to putenv() is already covered by POS34-C." The issue isn't that this rule lets you write insecure code; it's that it doesn't let you write safe code. It currently requires you to free memory that isn't safe to free.
David Svoboda
Signals are weird. ISO C standardizes signal() and raise(), but no other signal-related stuff. So SIG34-C's NCCE is actually ISO-compliant, but we can't discuss what the program does in general, because the way it behaves (badly) is platform-dependent. Hence the partitioning of examples into POSIX vs Windows sections.
Random numbers are similarly pathalogical. ISO C defines rand() and srand(), but they do not guarantee that rand() delivers sufficiently random numbers. So we forbid its use in MSC30-C. Meanwhile, when we first wrote MSC32-C it was about using srand() properly, as well as POSIX & Windows-specific solutions. So MSC32-C now describes a platform-neutral problem, but we can no longer write a platform-neutral NCCE for it without also violating MSC30-C.