CODEWITHSUNDEEP
c
num3ric
num3ric

Reputation: 696

Structs containing dynamic char arrays in C

I'm looking for the right way to allocate and deallocate chars arrays of dynamic sizes contained inside structs (an array of "Command" structs in fact). Is the following correct way to approach this problem in C? I was having many problems before I realized I had to NULL all the array entries at the start. If I don't, I get the "pointer being freed was not allocated" error.

Note that I have to free the Command struct data because I'm reusing them in a loop.

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

#define MAX_NB_ARGS 10

typedef struct {
    char* args[MAX_NB_ARGS];
} Command;

void init_commands(Command commands[2]) {
    int i;
    for (i=0; i<2; ++i) {
        int j;
        for (j=0; j<MAX_NB_ARGS; ++j) {
            // If I don't null these, the free_commands
            // function tries to free unallocated memory
            // and breaks
            commands[i].args[j] = NULL;
        }
        // Constant of 3 here but in reality variable size
        // Idem for the size of the "args" string
        for (j=0; j<3; ++j) {
            commands[i].args[j] = malloc(5*sizeof(char));
            strcpy(commands[i].args[j], "test");
        }
    }
}

void free_commands(Command commands[2]) {
    int i;
    for (i=0; i<2; ++i) {
        int j=0;
        // Do I really clear the second command?
        // Debugger shows every args as nil on
        // on the second iteration.
        while (commands[i].args[j]) {
            free(commands[i].args[j]);
            commands[i].args[j] = NULL;
            ++j;
        }
    }
}

int main () {
    Command commands[2];
    int i=0;
    // Command structs are being reused within the loop
    while ((i++)<2) {
        init_commands(commands);
        puts(commands[1].args[2]); //test print. works.
        free_commands(commands);
    }
    return 0;
}

Upvotes: 1

Views: 1085

Answers (2)

Jonathan Leffler
Jonathan Leffler

Reputation: 755026

When I run the code under valgrind on MacOS X 10.7.2, the code compiles and runs cleanly, and the only memory left in use is, I believe, system memory. Nothing appears to be leaked.

This agrees with my eyeballing of the code; I don't see a leak in the code.

If I add appropriate printf() statements around the malloc() and free() calls, I get:

allocating (0,0) <<test>>
allocating (0,1) <<test>>
allocating (0,2) <<test>>
allocating (1,0) <<test>>
allocating (1,1) <<test>>
allocating (1,2) <<test>>
test
freeing (0,0) <<test>>
freeing (0,1) <<test>>
freeing (0,2) <<test>>
freeing (1,0) <<test>>
freeing (1,1) <<test>>
freeing (1,2) <<test>>
allocating (0,0) <<test>>
allocating (0,1) <<test>>
allocating (0,2) <<test>>
allocating (1,0) <<test>>
allocating (1,1) <<test>>
allocating (1,2) <<test>>
test
freeing (0,0) <<test>>
freeing (0,1) <<test>>
freeing (0,2) <<test>>
freeing (1,0) <<test>>
freeing (1,1) <<test>>
freeing (1,2) <<test>>

Your code is clean. But learn how to do debug printing like this.

Yes, you absolutely need to ensure that your memory is initialized before you read it. You can manage this by keeping a track of which entries are in use because you've written a valid pointer to them (you'd need an extra member in the structure to do so, presumably), or by marking the unused ones explicitly with NULL (so you've written to those, too). Both techniques work; one or the other must be used since the new memory that is returned by malloc() and realloc() (and, theoretically, though not in practice, the new memory that is returned by calloc()) is not properly initialized to null.

(Why calloc() and theory? Well, in theory a null pointer need not be all bytes zero, but calloc() sets memory to all bytes zero. However, I've not come across a machine where a NULL pointer is not all bytes zero - and lots of code would run into lots of problems on such a machine. So, in practice, calloc() returns nulled memory, but in theory, there could be a machine where it does not do so.)

Upvotes: 0

fdk1342
fdk1342

Reputation: 3564

The reason you crash in free_commands is because the argument to the function is a stack variable which is not initialized. Therefore the array args[] contained uninitialized pointer values. So, yes you are required to initialize them to NULL for free_commands to work properly, otherwise you will call free() with garbage pointers. To avoid a memory leak you need to call free() for every malloc().

As for your other comment you do need to run through every argument of every command. However your loop structure is incorrect. Don't use a while loop for the inner loop you might advance past the arguments from command 0 into the arguments from command 1 or potentially outside of the Commands array all together (that will happen if every argument of every command was assigned a memory block) . It should be a for loop starting from 0 to MAX_NB_ARGS. Its safe to call free() with a NULL pointer, or you could break out of the inner loop early if NULL is detected.

Also don't forget to check the return value of malloc().

Upvotes: 1

Related Questions