Handling clean-up in function with multiple allocations

Question

Say I have a function that allocates some struct (object) and has to allocate additional data when setting its fields. Since each allocation can fail, the object creation may only partly succeed. What is the most common way of cleaning this mess up? I suppose the most straight forward thing would be something like this:

struct Obj {
    struct ChildObj1 *child_obj1;
    struct ChildObj2 *child_obj2;
};

struct Obj *obj_create()
{
    struct Obj *obj = malloc(sizeof(struct Obj));
    if (!obj) {
        return NULL;
    }

    obj->child_obj1 = child_obj1_create();
    if (!obj->child_obj1) {
        free(obj);
        return NULL;
    }

    obj->child_obj2 = child_obj2_create();
    if (!obj->child_obj2) {
        free(obj->child_obj1);
        free(obj);
        return NULL;
    }

    return obj;
}

I feel like this could get pretty tedious if there are a lot of objects to be created. Plus you may easily forget to update the "free unwind" when you make changes later on. An alternative I'm pondering is to utilize the "destructor" I'd need anyway, but make sure to zero initialize the struct. Like so:

struct Obj *obj_create()
{
    struct Obj *obj = malloc(sizeof(struct Obj));
    if (!obj) {
        return NULL;
    }
    memset(obj, 0, sizeof(struct Obj));

    obj->child_obj1 = child_obj1_create();
    if (!obj->child_obj1) {
        obj_destroy(obj);
        return NULL;
    }

    obj->child_obj2 = child_obj2_create();
    if (!obj->child_obj2) {
        obj_destroy(obj);
        return NULL;
    }

    return obj;
}

void obj_destroy(struct Obj *obj)
{
    if (!obj)
        return;

    if (obj->child_obj2)
        free(obj->child_obj2);

    if (obj->child_obj1)
        free(obj->child_obj1);

    free(obj);
}

Are there any problems with this approach? And are there any other better alternatives out there?

dbush · Accepted Answer · 2024-10-13 21:44:09Z

5

This is one of the rare cases where it makes sense to use goto.

You can use it to jump forward to a set of error cases, with the cleanups happening in the reverse order of the allocations.

struct Obj *obj_create()
{
    struct Obj *obj = malloc(sizeof(struct Obj));
    if (!obj) {
        goto err1;
    }

    obj->child_obj1 = child_obj1_create();
    if (!obj->child_obj1) {
        goto err2;
    }

    obj->child_obj2 = child_obj2_create();
    if (!obj->child_obj2) {
        goto err3;
    }

    return obj;

err3:
    free(obj->child_obj1);
err2:
    free(obj);
err1:
    return NULL;       
}

answered Oct 13, 2024 at 21:44

dbush

233k27 gold badges261 silver badges334 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

ShadowRanger Over a year ago

For slightly simpler code, you could initialize things to NULL, so you only need a single label to jump to. calloc the obj, and add if (obj) before calling free(obj->child_obj1);. Not necessarily useful for a one-off, but codebases I've worked with used this approach uniformly, hiding the "evil" goto in a macro that expanded to goto _CLEANUP , and additional wrapping macros to simplify checking the result and dumping logs on each possible failure location.

Ted Lyngmo · Accepted Answer · 2024-10-13 21:43:45Z

4

It's pretty common to use goto to jump to an error section at the end of the function where all the successful allocations are free'd. Example:

struct Obj *obj_create() {
    struct Obj *obj = malloc(sizeof *obj);
    if (!obj) return NULL;

    obj->child_obj1 = child_obj1_create();
    if (!obj->child_obj1) goto free1;

    obj->child_obj2 = child_obj2_create();
    if (!obj->child_obj2) goto free2;

    return obj;
    
free2:
    free(obj->child_obj1);
free1:
    free(obj);
    return NULL;
}

answered Oct 13, 2024 at 21:43

Ted Lyngmo

124k7 gold badges93 silver badges155 bronze badges

2 Comments

Andreas Wenzel Over a year ago

Instead of duplicating the return NULL; statement, in my opinion, it is cleaner to use goto also in the first case, for consistency. This was done in one of the other answers. However, that is probably a matter of taste.

Ted Lyngmo Over a year ago

@AndreasWenzel True. If one wants to add logging to the error case, I would definitely jump to the end even when no successful allocation has been made, but for this I think I prefer returning directly, but I don't have a strong opinion about it. Either goes.

Vlad from Moscow · Accepted Answer · 2024-10-14 14:58:54Z

1

There is no any need to use the goto statement. And in general forget the goto statement.:) And also try to use one return statement from a function if possible:) Numerous return statements make a function less readable with redundant and duplicated code.

Instead you can write for example

struct Obj * obj_create( void )
{
    struct Obj *obj = NULL;

    if ( ( obj = malloc(sizeof(struct Obj) ) != NULL )
    {
        if ( !(obj->child_obj1 = child_obj1_create() ) ||
             !(obj->child_obj2 = child_obj2_create() ) )
        {
            free( obj->child_obj1 );
            free( obj );

            obj = NULL;
        }
    }
   
    return obj;
}

The function free may be called for a null pointer. Or instead of this statement

free( obj->child_obj1 );

you could write if you want

if ( obj->child_obj1 ) free( obj->child_obj1 );

As you can see the function is compact and clear.

Within the function you can use your function obj_destroy that should be rewritten again without numerous return statements like

void obj_destroy( struct Obj *obj )
{
    if ( obj )
    {
        if ( obj->child_obj2 ) free( obj->child_obj2 );
        if ( obj->child_obj1 ) free( obj->child_obj1 );

        free(obj);
    }
}

the following way

struct Obj * obj_create( void )
{
    struct Obj *obj = NULL;

    if ( ( obj = malloc(sizeof(struct Obj) ) != NULL )
    {
        obj->child_obj2 = NULL;             

        if ( !(obj->child_obj1 = child_obj1_create() ) ||
             !(obj->child_obj2 = child_obj2_create() ) )
        {
            obj_destroy( obj );

            obj = NULL;
        }
    }
   
    return obj;
}

I'll demonstrate why the approach with the goto statement should be thrown in the trash.:)

Let's assume that requirements for the task was changed and now instead of the two data members of the pointer types you have to use an array of pointers of one pointer type like

enum { N = 5 };

struct Obj {
   struct ChildObj *child_obj[N];
};

In this case if to use the goto statement freeing the already allocated memory will look something like

err6:
    free(obj->child_obj[3]);
err5:
    free(obj->child_obj[2]);
err4:
    free(obj->child_obj[1]);
err3:
    free(obj->child_obj[0]);
err2:
    free(obj);
err1:
    return NULL;

Do you like such a code?:)

Actually the function in this case can look the following way based on the logic and the structure of the original function shown above:

struct Obj * obj_create( void )
{
    struct Obj *obj = NULL;

    if ( ( obj = malloc(sizeof(struct Obj) ) != NULL )
    {
        int i = 0;

        while ( i < N && ( obj->child_obj[i] = child_obj_create() ) != NULL )
        {
            ++i;
        }

        if ( i != N )
        {
            while ( i-- != 0 ) free( obj->child_obj[i] );
            free( obj );

            obj = NULL;
        }
    }
   
    return obj;
}

As you can see its structure and logic are similar to the structure and logic of the first original function shown by me. In fact this if statement in the original function

if ( !(obj->child_obj1 = child_obj1_create() ) ||
     !(obj->child_obj2 = child_obj2_create() ) )

simulates the while loop in the preceding function. The only difference is that there is no used an index because the original structure has no array. But the logic and the structure of the original function are preserved in the modified function.:)

And this single statement

free( obj->child_obj1 );

substitutes for this while loop in the modified function

while ( i-- != 0 ) free( obj->child_obj[i] );

Or vice versa if you had the modified function and needed to use two data members of different types instead of an array you can easily get the original function with the same logic and without any goto statement.:)

If to use within the function the function obj_destroy that in this case will look the following way

void obj_destroy( struct Obj *obj )
{
    if ( obj )
    {
        for ( int i = 0; i < N && obj->child_obj[i] != NULL; i++ )
        {
            free( obj->child_obj[i] );
        }
 
        free(obj);
    }
}

then the function obj_create will look as

struct Obj * obj_create( void )
{
    struct Obj *obj = NULL;

    if ( ( obj = malloc(sizeof(struct Obj) ) != NULL )
    {
        int i = 0;

        while ( i < N && ( obj->child_obj[i] = child_obj_create() ) != NULL )
        {
            ++i;
        }

        if ( i != N )
        {
            obj_destroy( obj );

            obj = NULL;
        }
    }
   
    return obj;
}

Note: On the other hand, I would like to make a remark. It is unclear why together with allocating memory referenced by the pointer obj you are also trying to allocate memory for data members obj->child_obj1 and obj->child_obj2. Maybe you should write

struct Obj * obj_create( void )
{
    struct Obj *obj = malloc( sizeof( struct Obj ) );

    if ( obj != NULL )
    {
        obj->child_obj1 = NULL;
        obj->child_obj2 = NULL;
    }
   
    return obj;
}

and allocate memory referenced by the data members obj->child_obj1 and obj->child_obj2 separately when it will be required for each data member.

edited Oct 14, 2024 at 14:58

answered Oct 13, 2024 at 22:02

Vlad from Moscow

313k27 gold badges204 silver badges358 bronze badges

11 Comments

Andrew Henle Over a year ago

Except - multiple assignments in if statements - probably the most bug-prone construct available in C. goto is IMO much preferable.

Vlad from Moscow Over a year ago

@AndrewHenle I can't agree with you. :) The code is enough clear and simple. Using goto statements is usually error-prone and the corresponding code is difficult to modify.

ShadowRanger Over a year ago

Why the if ( obj != NULL ) and NULLing instead of just using calloc as a one-liner?

Vlad from Moscow Over a year ago

@ShadowRanger As far as I know it is not necessary that a null pointer contains all zeroes. It is implementation defined,

ShadowRanger Over a year ago

@VladfromMoscow: I suppose. I'm not sure I've ever worked on a system that violates that expectation, and I've worked on some weird systems, but sure, technically true.

|

Lundin · Accepted Answer · 2024-10-14 07:50:32Z

When one malloc fails, it's not really important to clean everything up. At that point the only sensible thing you can do on the caller side is to close down gracefully, showing error messages etc. The only reason we don't call exit directly is just to allow the caller to report errors.

So in this specific case, I wouldn't over-engineer the code:

struct Obj *obj_create (void)
{
    struct Obj *obj = malloc(sizeof(struct Obj));
    if (obj == NULL) { return NULL; }

    obj->child_obj1 = child_obj1_create();
    if (obj->child_obj1 == NULL) { return NULL; }

    obj->child_obj2 = child_obj2_create();
    if (obj->child_obj2 == NULL) { return NULL; }

    return obj;
}

Yes, normally it is good practice to always free() up everything because that way you got notified of heap-related bugs related to allocation/pointers - at the point when you call free() and get a crash, you have one such bug elsewhere.

But if improved error diagnostic was desired, you wouldn't return an object but an error code anyway, and instead return the object through a parameter.

Collectives™ on Stack Overflow

Handling clean-up in function with multiple allocations

4 Answers 4

1 Comment

2 Comments

11 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

2 Comments

11 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related