Freeing a 2D array of malloc'd strings fails in C

Question

I know this is a question that has been asked quite frequently but I have read 10+ closed questions without any luck as my solution seems to match those proposed as solutions by others.

I am writing my own shell as a learning exercise and in doing so, I am malloc'ing an array of strings to serve as the arguments of a program call. My malloc is done as follows where argcnt is the number of arguments given + 2 to match the standard argv size and allow the array to be null terminated and thus used by execvp:

char ** args;
args = (char **) malloc(argcnt*sizeof(char *));
for(i = 0; i < argcnt; i++) {
    args[i] = (char *) malloc(80*sizeof(char));
    printf("Made %d\n", i);
}

I then free the same memory as follows:

for (i = 0; i < argcnt; i++) {
    free(args[i]);
    printf("Freed %d\n", i);
}
free(args);

The program compiles but fails at freeing argv[1] at runtime. The sample output of running the program and then calling ls -a -l is as follows:

jack@ubuntu:~/myshell$ ./myshell 
[30/03 23:34] # ls -a -l
Argcnt: 4
Made 0
Made 1
Made 2
Made 3
Arg[0]: ls
Arg[1]: -a
Arg[2]: -l
Arg[3]: (null)
Freed 0
*** Error in `./myshell': free(): invalid pointer: 0x0000000001a84c43 ***
Aborted (core dumped)

I have been wrestling with this for the last 2 hours and can't work out what is wrong so some insight into the problem would be very much appreciated.

EDIT: The function currently breaking my program is:

void breakargs(char * cmd, char ** args) {
    char * tok = malloc(strlen(cmd)*sizeof(char)); //maximum token size is full cmd
    char * str = malloc(strlen(cmd)*sizeof(char));
    int i=1;
    strcpy(str, cmd); //maintains integrity of cmd

    args[0] = tok = strtok(str, " ");
    while (tok != NULL)
    {
        args[i] = tok = strtok(NULL, " ");
        i++;
    }
    args[i] = '\0';
    free(tok);
}

2nd EDIT: The problem was my reassigning of the pointers made by my original malloc with strtok so that the original pointer reference was lost. Furthermore, my use of a definite string length for arguments could potentially lead to problems. The solution was to only malloc args[i] when I knew the length of the string that needs to be stored there, and then move the string into that memory location using strcpy instead of direct assignment as I had been doing. This maintains the integrity of the pointer reference and allows it to be freed correctly.

Answer 1

The problem isn't in the code you show; it is in the code you don't show. Without having read the complete comment chain, I suspect you're doing something similar to this:

 args[0] = "ls";
 args[1] = "-a";
 args[2] = "-l";
 args[3] = NULL;

and then trying to free the args array. Maybe your code was not quite so blatant, but the net result may be much the same. For example, you could have split up a command line that was read into a buffer and might copy the pointers to sections of the buffer into the args array elements. (Update: This seems to be the actual problem.)

This has obliterated the allocated memory (you've lost it — you've leaked memory), and later you're trying to free unallocated memory (pointers that were not returned by malloc() or one of its friends). The first such free() often 'works', but completely messes up the controls used by malloc() et al so that the second free() fails. Many, but not all, modern systems sport a malloc() that detects many such abuses.

You need code more like:

strcpy(args[0], "ls");
strcpy(args[1], "-a");
strcpy(args[2], "-l");
free(args[3]);
args[3] = NULL;

Note that allocating 80 bytes per string was a gross over-allocation for these strings. OTOH, it could also be completely inadequate for other (longer) arguments. You should allocate the memory for args itself as you did; you should probably allocate the memory for the individual elements according to what the corresponding argument contains. There's a (POSIX but not Standard C) function called strdup() that might be what you're looking for — it allocates enough space for a copy of a string and copies the string into the allocated space. Alternatively, you might not need the allocations at all.

If it is available for your platform, you should use valgrind to validate your memory use and identify your abuses.

Answer 2

strtok returns a pointer into the string that was initially passed to strtok, there is no need to allocate space for that pointer.

When you assign the return value of strtok to a pointer variable, you overwrite that variable's pointer value with the new value. This means, if that pointer variable points to memory you have already allocated, that memory will be "leaked" as you no longer have a pointer to it.

In short - if you allocate memory for a pointer variable, don't assign a different value to that pointer variable unless you have already freed the memory or put the pointer value somewhere else.

In your breakArgs function there are a number of issues:

void breakargs(char * cmd, char ** args) {
    char * tok = malloc(strlen(cmd)*sizeof(char)); //maximum token size is full cmd

You don't need to allocate memory for tok as you will assigning it a value from strtok

    char * str = malloc(strlen(cmd)*sizeof(char));
    int i=1;
    strcpy(str, cmd); //maintains integrity of cmd

    args[0] = tok = strtok(str, " ");

Don't assign directly into args[0] as that will overwrite the pointer to the memory you already allocated. Instead assign into tok, then strcpy(args[0], tok);

    while (tok != NULL)
    {
        args[i] = tok = strtok(NULL, " ");

Don't assign directly into args[i] as that will overwrite the pointer to the memory you already allocated. Instead assign into tok, then strcpy(args[i], tok); You should also check for tok being NULL before you strcpy from it.

        i++;
    }
    args[i] = '\0';

Don't assign directly into args[i], instead you could indicate the end by strcpy(args[i], "") so you have a blank string at the end.

    free(tok);

Don't free tok here, as tok should be NULL by this stage, you do need to free (str) though.

}

Also take note of some of the other comments about checking that the arguments you process don't exceed the limit you have put on the memory (e.g. 80 characters per item).