How to free some dynamically allocated nested array of structure in C?

Question

Here are some nested structures:

struct bat
{
    int match,run,fifty,best;
    double average,strike_rate;
};
struct ball
{
    char *best;
    int match,wicket,fiveW;
    double economy,average;
};
struct Player
{
    char *name;
    char *country;
    char *role;
    struct bat *batting;
    struct ball *bowling;

};
struct Team
{
    char *name;
    char *owner;
    char *rank;
    char *worth;
    char *match;
    char *won;
    char *lost;
    struct Player *plist;
} *team;

Below I dynamically allocated an array of 7 struct Team type using *team pointer each of which contains 16 struct Player type array using *plist. struct Player has also two nested structures.

int i,j;
    team=(struct Team *) calloc(7,sizeof(struct Team));
    for(i=0; i<7; i++)
    {
        (team+i)->name=( char*) malloc(1*100);
        (team+i)->owner=( char*) malloc(1*100);
        (team+i)->rank=( char*) malloc(1*100);
        (team+i)->worth=( char*) malloc(1*100);
        (team+i)->match=( char*) malloc(1*100);
        (team+i)->won=( char*) malloc(1*100);
        (team+i)->lost=( char*) malloc(1*100);
        (team+i)->plist=(struct Player *) calloc(16,sizeof(struct Player));
        for(j=0; j<16; j++)
        {
            (((team+i)->plist)+j)->name=( char*) malloc(1*100);
            (((team+i)->plist)+j)->country=( char*) malloc(1*100);
            (((team+i)->plist)+j)->role=( char*) malloc(1*100);
            (((team+i)->plist)+j)->batting=(struct bat *) malloc(sizeof(struct bat));
            (((team+i)->plist)+j)->bowling=(struct ball *) malloc(sizeof(struct ball));
            ((((team+i)->plist)+j)->bowling)->best=(char*) malloc(1*100);
        }
    }

Now I have assigned values to all of these and done some tasks. It's time to free all those dynamically allocated arrays. What is the correct way to free everything allocated above?

I tried to free like below but the program fetches run-time error and crashes:

for(i=0; i<7; i++)
    {
        free((team+i)->name);
        free((team+i)->owner);
        free((team+i)->rank);
        free((team+i)->worth);
        free((team+i)->match);
        free((team+i)->won);
        free((team+i)->lost);
        for(j=0; j<16; j++)
        {
            free((((team+i)->plist)+j)->name);
            free((((team+i)->plist)+j)->country);
            free((((team+i)->plist)+j)->role);
            free((((team+i)->plist)+j)->batting);
            free(((((team+i)->plist)+j)->bowling)->best);
            free((((team+i)->plist)+j)->bowling);
        }
        free(((team+i)->plist));
    }
    free(team);

How to free all those dynamically allocated memory correctly?

Free every pointer you `malloc()`ated, in the reverse order of allocation. — The Paramagnetic Croissant, Mar 31 '16 at 16:43
Considering most of those internal member `malloc` are fixed length hard coded into the source, I'm struggling to see why you're even bothering doing *any* of that, and rather just having sized members like `char name[100]` in the first place. Dynamic allocation: Jut because you *can* doesn't mean you *should*. — WhozCraig, Mar 31 '16 at 16:47
also, those parentheses are ugly. `(team+q)->plist)+b)->bowling)->best` should be written as `team[q].plist[b].bowling->best`. — The Paramagnetic Croissant, Mar 31 '16 at 16:48
If you are going to initialize all members of the `struct` there is no point in using `calloc` instead of `malloc`. — David Ranieri, Mar 31 '16 at 16:55
Suggest having a `team_init`/`team_cleanup` and `player_init`/`player_cleanup` functions, then you can simply loop; `(for 0 = 0; i < 7; i++) { team_init(team + i);` — Mark Nunberg, Mar 31 '16 at 17:04

score 3 · Answer 1 · answered Mar 31 '16 at 17:17

I put up some notes first, some of them from the comments:

Do not cast the result of malloc/calloc. This might hide errors / warnings when you forget to include stdlib.h.
sizeof(char) is 1. Always.
Using calloc when later initializing all of the allocated memory is nonsense: Just use malloc.
((((team+q)->plist)+b)->bowling)->best is ugly to read. Better use array notation:
```
team[q]->plist[b]->bowling->best
```
Though it doesn't buy much in terms of readability here, IMO.
q and b are not meaningful variable names.
If your strings are fixed length, then why do you bother using dynamic allocation at all? Just use fixed length array members, like char name[100]. Same goes for plist: If you know that it'll be 16 players, then just make it a constant sized array.

Then, you should probably have both construction and destruction functions for each of your structure, mimicking constructors and destructors from object oriented languages (since you're programming in an almost object oriented style). So consider writing functions

void construct_team(struct Team * team) {
  // allocating any members, calling construct_* of structure members
}
void destruct_team(struct Team * team) {
  // free any members, calling destruct_* of structure members, in
  // the exact reversed order as in the construct function!
}

That way, you would have noticed that you have no "matching allocation" for

free((team+q)); // Never allocated team+q (the individual array elements), just the whole array team!

You just need to free(team) at the end of your loop in order to make your code work. But please, please consider above remarks.

Such a verbose and usefull answer - yet not accepted as answer and only 2 votes (one of them from me). Stackoverflow is strange sometimes. — Gewure, Mar 07 '17 at 16:04

Ian Abbott · Answer 2 · 2016-03-31T17:36:47.063

2

These lines are wrong:

        free(((team+q)->plist)+q);
        free((team+q));
    }

They should be:

        free((team+q)->plist);
    }
free(team);

NOTE: I think @Tavij's mistake is thinking that each element of a calloced array has to be freed individually, which is, of course, incorrect.

edited Mar 31 '16 at 17:36

answered Mar 31 '16 at 17:21

Ian Abbott

15,083
19
33

Notice that this code is executed inside a loop and `team` is an array, you can not ommit the `+q`. – David Ranieri Mar 31 '16 at 17:23
@AlterMann My initial version had incorrect indentation. I fixed it, so maybe it makes more sense now. The final `free(team);` is after the `for` loop. – Ian Abbott Mar 31 '16 at 17:25

How to free some dynamically allocated nested array of structure in C?

2 Answers2