C Language Common errors


Example

Improper use of pointers are frequently a source of bugs that can include security bugs or program crashes, most often due to segmentation faults.

Not checking for allocation failures

Memory allocation is not guaranteed to succeed, and may instead return a NULL pointer. Using the returned value, without checking if the allocation is successful, invokes undefined behavior. This usually leads to a crash, but there is no guarantee that a crash will happen so relying on that can also lead to problems.

For example, unsafe way:

struct SomeStruct *s = malloc(sizeof *s);
s->someValue = 0; /* UNSAFE, because s might be a null pointer */

Safe way:

struct SomeStruct *s = malloc(sizeof *s);
if (s)
{
    s->someValue = 0; /* This is safe, we have checked that s is valid */
}

Using literal numbers instead of sizeof when requesting memory

For a given compiler/machine configuration, types have a known size; however, there isn't any standard which defines that the size of a given type (other than char) will be the same for all compiler/machine configurations. If the code uses 4 instead of sizeof(int) for memory allocation, it may work on the original machine, but the code isn't necessarily portable to other machines or compilers. Fixed sizes for types should be replaced by sizeof(that_type) or sizeof(*var_ptr_to_that_type).

Non-portable allocation:

 int *intPtr = malloc(4*1000);    /* allocating storage for 1000 int */
 long *longPtr = malloc(8*1000);  /* allocating storage for 1000 long */

Portable allocation:

 int *intPtr = malloc(sizeof(int)*1000);     /* allocating storage for 1000 int */
 long *longPtr = malloc(sizeof(long)*1000);  /* allocating storage for 1000 long */

Or, better still:

 int *intPtr = malloc(sizeof(*intPtr)*1000);     /* allocating storage for 1000 int */
 long *longPtr = malloc(sizeof(*longPtr)*1000);  /* allocating storage for 1000 long */

Memory leaks

Failure to de-allocate memory using free leads to a buildup of non-reusable memory, which is no longer used by the program; this is called a memory leak. Memory leaks waste memory resources and can lead to allocation failures.

Logical errors

All allocations must follow the same pattern:

  1. Allocation using malloc (or calloc)
  2. Usage to store data
  3. De-allocation using free

Failure to adhere to this pattern, such as using memory after a call to free (dangling pointer) or before a call to malloc (wild pointer), calling free twice ("double free"), etc., usually causes a segmentation fault and results in a crash of the program.

These errors can be transient and hard to debug – for example, freed memory is usually not immediately reclaimed by the OS, and thus dangling pointers may persist for a while and appear to work.

On systems where it works, Valgrind is an invaluable tool for identifying what memory is leaked and where it was originally allocated.

Creating pointers to stack variables

Creating a pointer does not extend the life of the variable being pointed to. For example:

int* myFunction() 
{
    int x = 10;
    return &x;
}

Here, x has automatic storage duration (commonly known as stack allocation). Because it is allocated on the stack, its lifetime is only as long as myFunction is executing; after myFunction has exited, the variable x is destroyed. This function gets the address of x (using &x), and returns it to the caller, leaving the caller with a pointer to a non-existent variable. Attempting to access this variable will then invoke undefined behavior.

Most compilers don't actually clear a stack frame after the function exits, thus dereferencing the returned pointer often gives you the expected data. When another function is called however, the memory being pointed to may be overwritten, and it appears that the data being pointed to has been corrupted.

To resolve this, either malloc the storage for the variable to be returned, and return a pointer to the newly created storage, or require that a valid pointer is passed in to the function instead of returning one, for example:

#include <stdlib.h>
#include <stdio.h>

int *solution1(void) 
{
    int *x = malloc(sizeof *x);
    if (x == NULL) 
    {
        /* Something went wrong */
        return NULL;
    }

    *x = 10;

    return x;
}

void solution2(int *x) 
{
    /* NB: calling this function with an invalid or null pointer 
       causes undefined behaviour. */

    *x = 10;
}

int main(void) 
{
    { 
        /* Use solution1() */

        int *foo = solution1();  
        if (foo == NULL)
        {
            /* Something went wrong */
            return 1;
        }

        printf("The value set by solution1() is %i\n", *foo);
        /* Will output: "The value set by solution1() is 10" */

        free(foo);    /* Tidy up */
    }

    {
        /* Use solution2() */

        int bar;
        solution2(&bar); 

        printf("The value set by solution2() is %i\n", bar);
        /* Will output: "The value set by solution2() is 10" */
    }

    return 0;
}

Incrementing / decrementing and dereferencing

If you write *p++ to increment what is pointed by p, you are wrong.

Post incrementing / decrementing is executed before dereferencing. Therefore, this expression will increment the pointer p itself and return what was pointed by p before incrementing without changing it.

You should write (*p)++ to increment what is pointed by p.

This rule also applies to post decrementing: *p-- will decrement the pointer p itself, not what is pointed by p.