Do I have to free any previously allocated memory after an error occurs?

Question

I'm writing a multithreaded server in C for a uni project and I'm having a hard time figuring out how to do error handling in a nice, readable and standard way.

Right now, if the program terminates successfully, I free every bit of allocated memory at the end of it. But what if a fatal error occurs during the execution (e.g. malloc returns NULL)?

For example, let's say I have a custom data type mydata_t and a constructor function mydata_t *mydata_init() that is used by several modules of my program. After seeing some code on the internet, this is how I would have written it:

mydata_t *mydata_init() {

    mydata_t *mydata = malloc(sizeof(mydata_t));
    if (!mydata) return NULL;

    mydata->field1 = malloc(sizeof(mydata2_t));
    if (!mydata->field1) return NULL;

    mydata->field2 = malloc(sizeof(mydata3_t));
    if (!mydata->field2) return NULL;

    /*
    Initialization of other fields
    */

    return mydata;

}

It does seem nice and clean, but is this the "standard" way to do it?

In particular, what if one of the mallocs returns NULL? Is it necessary to free all the previously allocated memory? Is it reasonable to change the code to something like this?

mydata_t *mydata_init() {

    mydata_t *mydata = malloc(sizeof(mydata_t));
    if (!mydata) goto err_1;

    mydata->field1 = malloc(sizeof(mydata2_t));
    if (!mydata->field1) goto err_2;

    mydata->field2 = malloc(sizeof(mydata3_t));
    if (!mydata->field2) goto err_3;

    /*
    Initialization of other fields
    */

    return mydata;

/*
Other tags
*/

err_3:
    free(mydata->field1);
err_2:
    free(mydata);
err_1:
    return NULL;

}

Answer 1

The existing answers covers the various malloc/free scenarios, so I will not add to that.

What I would like to point out is that if you fail a malloc, it is pretty much game-over for the program, and it is often better to just exit() than to try to recover the situation.

Even if you manage to clean up partially allocated structs, other code, and libraries used by that code, will also fail to allocate memory and may (and often will) fail in mysterious ways.

malloc() usually only fails if you run in severely restricted environments, like embedded micro controllers), or if you are leaking memory like a sieve.

Answer 2

Assuming a non-trivial OS, that's one with 'kill-9' or a Task Manger with 'End Process' entry on a GUI context menu, please bear in mind the following points before embarking on a lenghty and expensive campaign to write specific user code to free all malloced/whatever memory upon a fatal error:

1) Freeing all memory with user code requires more user code. That extra code must be designed, coded tested and maintained, often repeatedly after changes and/or new versions. With a complex, multithreaded app, maybe with pools of objects that are shared and communicated between threads, it's not going to be a remotely trivial exercise to even try to shut it down with user code.

2) Freeing all memory with user code after a fatal error can make things worse. If the error is a result of a corrupted heap manager, then you will raise even more faults as you try to free it. An app 'diasppearing' on a customer with an error log entry is bad enough, a screen full of AV error boxes and a stuck app is much worse.

3) Safely freeing all memory with user code can only be done by a thread if all other threads have been stopped so that they cannot possibly access any of that memory. Reliably stopping all process threads can only be done by the OS at process termination. User code cannot be guaranteed to do it - if a thread is stuck executing a lengthy operation in a library, you cannot reliably stop it. If you try, it may, for instance, leave the memory-manager locked. Just unblocking threads stuck on an I/O operation is difficult enough, often requiring bodges like opening a connection on the local network stack to force an accept() call to return early.

4) The OS 'terminate process' API, and all that is involved in it, has been tested a LOT. It works, and it comes free with your OS. Your user code that tries to stop threads and free memory will never accumulate as much testing.

5) User code that tries to stop threads and free memory is redundant - the OS does the same job, only better, quicker and more reliably. You are trying to clean up memory from a sub-allocator that the OS is going to soon destroy and deallocate anyway.

6) Many OS and other commercial libraries have already given way to the inevitable and accept that they cannot safely have all their memory freed at shutdown without causing problems, especially with multithreaded apps. The library authors cannot do it reliably, neither can you.

Sure, manage your memory during a run in a controlled and sensible manner, freeing what you malloc as required, so as not to leak memory during the lifetime of the process.

If you encounter a fatal error, however, maybe try to write the details to a log file or make some other debugging/logging action, if you can, and then call your OS 'terminate process' API.

There is nothing else you can safely do.

Your app is near death, let the OS euthanize it.

Answer 3

Is it necessary to free all the previously allocated memory?

No, but non-leaky (good) code does.

You will find various opinions on how to do this (free up things), but the end goal is to do it, somehow. Free unused resources.

Code for clarity.

Note: goto form is an acceptable use of goto.

---

I'll offer another approach and use a mydata_uninit() companion function as possible.

mydata_t *mydata_uninit(mydata_t *mydata) {
  if (mydata) {
    free(mydate->field1); 
    mydate->field1 = NULL; // example discussed below **
    free(mydate->field2); 
    // free other resources and perform other clean up/accounting.
    free(mydate); 
  }
  return NULL;
}

I'd also allocate to the size of the de-referenced pointer, not the type.

mydata_t *mydata_init(void) {
  mydata_t *mydata = calloc(1, sizeof *mydata);
  if (mydata == NULL) {
    return NULL;
  }   
  mydata->field1 = calloc(1, sizeof *(mydata->field1));
  mydata->field2 = calloc(1, sizeof *(mydata->field2));
  // Other 1st time assignments

  // If any failed
  if (mydata->field1 == NULL || mydata->field2 == NULL) {
    mydata = mydata_uninit(mydata);
  }

  return mydata;
}

** Setting pointer struct members to NULL (mydata->field1) and then later freeing the struct pointer (mydata) I find aids in debug as errant code that de-references a NULL pointer typically errors faster than a free'd pointer.

Answer 4

One possible alternative.

mydata_t *mydata_init()
{
    mydata_t *mydata = malloc(sizeof(mydata_t));
    if (mydata == NULL)
    {
        /* Handle error */
        return NULL;
    }

    mydata->field1 = malloc(sizeof(mydata2_t));
    mydata->field2 = malloc(sizeof(mydata3_t));
    ...

    if (mydata->field1 != NULL && 
        mydata->field2 != NULL &&
        ...)
    {
        /* success */
        /*
         * Initialize everything
         */
        return mydata;
    }

    free(mydata->field1);
    free(mydata->field2);
    ...
    free(mydata);
    return NULL;
}

Note that you do not need to check for NULL before calling free() on the error path. As noted in the first answer to this question

Quoting the C standard, 7.20.3.2/2 from ISO-IEC 9899:

If ptr is a null pointer, no action occurs.