Skip to end of metadata
Go to start of metadata

Dynamic memory management is a common source of programming flaws that can lead to security vulnerabilities. Poor memory management can lead to security issues, such as heap-buffer overflows, dangling pointers, and double-free issues [Seacord 2013]. From the programmer's perspective, memory management involves allocating memory, reading and writing to memory, and deallocating memory.

Allocating and freeing memory in different modules and levels of abstraction may make it difficult to determine when and if a block of memory has been freed, leading to programming defects, such as memory leaks, double-free vulnerabilities, accessing freed memory, or writing to freed or unallocated memory.

To avoid these situations, memory should be allocated and freed at the same level of abstraction and, ideally, in the same code module. This includes the use of the following memory allocation and deallocation functions described in subclause 7.23.3 of the C Standard [ISO/IEC 9899:2011]:

void *malloc(size_t size);

void *calloc(size_t nmemb, size_t size);

void *realloc(void *ptr, size_t size);

void *aligned_alloc(size_t alignment, size_t size);
 
void free(void *ptr);

Failing to follow this recommendation has led to real-world vulnerabilities. For example, freeing memory in different modules resulted in a vulnerability in MIT Kerberos 5 [MIT 2004]. The MIT Kerberos 5 code in this case contained error-handling logic, which freed memory allocated by the ASN.1 decoders if pointers to the allocated memory were non-null. However, if a detectable error occurred, the ASN.1 decoders freed the memory that they had allocated. When some library functions received errors from the ASN.1 decoders, they also attempted to free the same memory, resulting in a double-free vulnerability.

Noncompliant Code Example

This noncompliant code example shows a double-free vulnerability resulting from memory being allocated and freed at differing levels of abstraction. In this example, memory for the list array is allocated in the process_list() function. The array is then passed to the verify_size() function that performs error checking on the size of the list. If the size of the list is below a minimum size, the memory allocated to the list is freed, and the function returns to the caller. The calling function then frees this same memory again, resulting in a double-free and potentially exploitable vulnerability.

enum { MIN_SIZE_ALLOWED = 32 };

int verify_size(char *list, size_t size) {
  if (size < MIN_SIZE_ALLOWED) {
    /* Handle error condition */
    free(list);
    return -1;
  }
  return 0;
}

void process_list(size_t number) {
  char *list = (char *)malloc(number);
  if (list == NULL) {
    /* Handle allocation error */
  }

  if (verify_size(list, number) == -1) {
      free(list);
      return;
  }

  /* Continue processing list */

  free(list);
}

The call to free memory in the verify_size() function takes place in a subroutine of the process_list() function, at a different level of abstraction from the allocation, resulting in a violation of this recommendation. The memory deallocation also occurs in error-handling code, which is frequently not as well tested as "green paths" through the code.

Compliant Solution

To correct this problem, the error-handling code in verify_size() is modified so that it no longer frees list. This change ensures that list is freed only once, at the same level of abstraction, in the process_list() function.

enum { MIN_SIZE_ALLOWED = 32 };

int verify_size(const char *list, size_t size) {
  if (size < MIN_SIZE_ALLOWED) {
    /* Handle error condition */
    return -1;
  }
  return 0;
}

void process_list(size_t number) {
  char *list = (char *)malloc(number);

  if (list == NULL) {
    /* Handle allocation error */
  }

  if (verify_size(list, number) == -1) {
      free(list);
      return;
  }

  /* Continue processing list */

  free(list);
}

Risk Assessment

The mismanagement of memory can lead to freeing memory multiple times or writing to already freed memory. Both of these coding errors can result in an attacker executing arbitrary code with the permissions of the vulnerable process. Memory management errors can also lead to resource depletion and denial-of-service attacks.

Recommendation

Severity

Likelihood

Remediation Cost

Priority

Level

MEM00-C

High

Probable

Medium

P12

L1

Automated Detection

Tool

Version

Checker

Description

CodeSonar
5.0p0

ALLOC.DF
ALLOC.LEAK

Double free
Leak

Compass/ROSE



Could detect possible violations by reporting any function that has malloc() or free() but not both. This would catch some false positives, as there would be no way to tell if malloc() and free() are at the same level of abstraction if they are in different functions

Coverity6.5RESOURCE_LEAKFully implemented
Klocwork
2018

FREE.INCONSISTENT
UFM.FFM.MIGHT
UFM.FFM.MUST
UFM.DEREF.MIGHT
UFM.DEREF.MUST
UFM.RETURN.MIGHT
UFM.RETURN.MUST
UFM.USE.MIGHT
UFM.USE.MUST
MLK.MIGHT
MLK.MUST
MLK.RET.MIGHT
MLK.RET.MUST
FNH.MIGHT
FNH.MUST
FUM.GEN.MIGHT
FUM.GEN.MUST
RH.LEAK


LDRA tool suite
9.7.1

50 D

Partially implemented

Parasoft C/C++test
10.4.2

CERT_C-MEM00-a
CERT_C-MEM00-b
CERT_C-MEM00-c
CERT_C-MEM00-d
CERT_C-MEM00-e

Do not allocate memory and expect that someone else will deallocate it later
Do not allocate memory and expect that someone else will deallocate it later
Do not allocate memory and expect that someone else will deallocate it later
Do not use resources that have been freed
Ensure resources are freed

Parasoft Insure++

Runtime analysis
Polyspace Bug Finder

R2018a

Invalid free of pointer

Deallocation of previously deallocated pointer

Use of previously freed pointer

Pointer deallocation without a corresponding dynamic allocation

Memory freed more than once without allocation


Memory accessed after deallocation

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

Related Guidelines

Bibliography

[MIT 2004]
[Plakosh 2005]
[Seacord 2013]Chapter 4, "Dynamic Memory Management"



19 Comments

  1. originally posted by by pete filandro at Aug 01, 2007 21:08

    One method of allocation recommended by the rabble on news:comp.lang.c is :

    #include <stdlib.h>

    ptr = malloc(NMEMB * sizeof *ptr);

    That, plus the related expression, (ptr = malloc(sizeof *ptr)), and also:

    ptr = malloc(string_length + 1);

    can take care of most malloc call situations.

  2. The narratives seem to assume that "Handle Error" does not include "return".  If that also applies to "Handle Allocation Error", then both examples (NCCE and CS) are vulnerable to trying to free NULL.  A quick check of "man 3 free" says that nothing happens, but I could swear I've seen that cause a crash.  Maybe that's implementation-defined, in which case it's still worth a mention.

    1. Some pre-C89 compilers (strictly, the libraries) objected to freeing null pointers (usually by crashing).

      All compilers/libraries compliant with C89 or later accept free(0) as a no-op.

  3. Pretty close. One point of information is that calling free on a null pointer is perfectly kosher; it does a noop.

    The first /* Handle Allocation Error */, which operates if malloc() fails is therefore free to do anything, including return, goto, longjmp, exit() ..., or none of these. If it does not exit, the free operates on a null pointer, which does nothing.

    The second /* Handle Error /, as well as the / Continue Processing */ are both constrained from any non-local exits. So one cannot do return, goto, longjmp, exit()..., without leaking the allocated memory.

  4. The realpath() function (defined in stdlib.h) takes a pathname and returns a canonicalized pathname. The point here is that it can (depending on the parameters) return its own malloc'ed result string, which must then be freed by the caller. Many other standard library functions that must return a new string have the option to allocate it and expect the user to free the result string when done with it. Doesn't this design pattern violate this recommendation?

    C++ can sidestep this issue with RAII. A function can return an object that malloc's a string upon creation and free's it upon destruction.

    I vaguely remember reading somewhere that the main reason Java implemented garbage collection was this particular problem; how do you handle malloc'ed data returned by a library function? and how do you know WHEN you are responsible for free()ing data returned by a library function?

    1. There should be no realpath() function declaration in <stdlib.h>.

      1. Under Standard C, there would be no realpath() in <stdlib.h>.  Under POSIX, there would.

    2. Doesn't this design pattern violate this recommendation?

      Yes, but as long as the responsibility for which function is responsible for freeing the allocated space is properly documented and then acted on, it is legitimate. I would suspect that one reason this is a recommendation rather than a rule is issues such as this.

      Also, according to the POSIX description of realpath():

      • If resolved_name is a null pointer, the behavior of realpath() is implementation-defined.

      I take it that the implementation you are familiar with documents the implementation-defined behaviour as "realpath() will allocate enough space for the path via malloc() and the caller is responsible for freeing the allocated space".

  5. This rule cannot be followed when dynamic allocation is used for linked data structures, which is probably its most important application.  A better rule is "have only one node freeing function and ensure that it maintains the integriry of the data structure."  It is also wise to use "handles" (volatile pointer to pointer to allocated chunk) that get NULLified by the freeing function.  Generally speaking, a lot of thought should go into the design.

  6. I disagree with the ROSE suggestion for this rule, what if a program has wrappers for malloc() and free()? The wrappers may be called within the same function, but each wrapper only calls malloc() OR free() accordingly.

    1. Alex and I discussed this today. I have removed the 'rose-possible' tag, but am not convinced that writing a rose rule is worthless...what do others think? Here is the conclusion that Alex and I came to:

      The tool Compass / ROSE does not currently detect violations of this recommendation, but it could. One merely has to search a function's local block for a call to either malloc() or free(), but not both. A function could have multiple calls to free() for each malloc() call, as the compliant solution illustrates. This would catch cases such as the non-compliant example above. However, there is a common coding style where malloc() is called inside an init function, and free() is called inside a destroy function, which mimicks C++ constructors and destructors. ROSE would flag these as false positives, and there is no good heuristic to train ROSE to identify and accept these patterns.

      1. Currently marked as rose-nonapplicable. How do LDRA and Fortify catch violations? How do they avoid false positives on malloc in an init function or free in a destroy function?

        This might be a good argument to have ROSE flag such rules, but only if in a 'sensitive' mode where false positives are less severe.

        1. Added back rose algo section, marked as 'rose-false-positive'.

  7. The advice here should apply equally well to other kinds of resources besides memory, including file pointers and descriptors, threads, processes, and synchronization primitives, etc. Would it make sense to generalize this guideline to encompass all types of resources or should there be a separate guideline for resources other than memory?

    1. Agreed. (I think we do have a rule about closing open files, in the FIO section somewhere.)

  8. verify_list() in the description should probably be verify_size().

  9. I removed a sentence (which looks to me redundant) from the beginning paragraph.

    Revert to the previous version if it lose the original intended meaning...