1

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;
}
5
  • if your plan is to use an array type, why encapsulating it in a struct type? Commented Feb 1, 2012 at 19:30
  • Have you got valgrind available (Linux - but also some other places). If so, use it. Commented Feb 1, 2012 at 19:30
  • @ouah I am simplifying the program here for clarity, the actual struct contains more information (another string and ints). Commented Feb 1, 2012 at 19:31
  • Except for the magic numbers on your array sizes (and number of chars to allocate), this is workable if you always want to allocate and free every command in the list. If you wanted to grow your list over time, you'd need a little more overhead. Commented Feb 1, 2012 at 19:34
  • 1
    Though it does not affect the working (or correctness) of the code (the i variable is not used) , the construct while ((i++)<2) {...} is IMHO a confusing idiom: the parentheses are not needed, and the loop could just as well have been written as for(i=1; i < 3; i++) {...}, which is much more standard (and thus:readable), IMHO. Commented Feb 1, 2012 at 19:47

2 Answers 2

1

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().

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

Comments

0

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.)

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.