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

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