Error on free char** table

Question

I have a function that takes a string and split it into tokens, because I want to return these tokens I allocate a variable using malloc.

char** analyze(char* buffer)
{
  int i= 0;
  char* token[512];

  char** final = (char**)malloc(strlen(buffer)+1);
  if ( final == NULL ) { perror("Failed to malloc"); exit(10); }

  token[i] = strtok(buffer, " ");
  while( token[i] != NULL )
  {
    final[i] = malloc(strlen(token[i])+1);
    if( final[i] == NULL ) { perror("Failed to malloc"); exit(11); }

    final[i] = token[i];

    i++;
    token[i] = strtok(NULL, " ");
   }

   final[i] = malloc(sizeof(char));
   if( final[i] == NULL ) { perror("Failed to malloc"); exit(12); }
   final[i] = NULL;

   return final;
}

And I try to free this table with another function:

void free_table(char** job)
{
  int i = 0;

  while( job[i] != NULL )
  {
    free(job[i]);
    i++;
  }
  free(job[i]); //free the last 
  free(job);
}

In main I use:

char** job = analyze(buffer); // buffer contains the string

and

free_table(job);

when I try to free the table I get this error:

*** Error in `./a.out': free(): invalid pointer: 0x00007ffd003f62b0 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7fdb2e5497e5]
/lib/x86_64-linux-gnu/libc.so.6(+0x7fe0a)[0x7fdb2e551e0a]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7fdb2e55598c]
./a.out[0x4012d6]

and the error goes on...

What am I doing wrong?

Answer 1

To begin with:

char** final = (char**)malloc(strlen(buffer)+1);

This allocates strlen(buffer) + 1 bytes, not that amount of "elements". And since sizeof(char*) is most likely very much larger than a single byte, you might be allocating to little memory here.

Since you don't know how many tokens there might be you should not allocate a fixed amount, but instead use realloc to reallocate as needed.

Then the second problem:

final[i] = malloc(strlen(token[i])+1);
...
final[i] = token[i];

In the first statement you allocate memory enough for the string pointed to by token[i], and assign the pointer to that memory to final[i]. But then you immediately reassign final[i] to point somewhere else, some memory that you haven't gotten from malloc. You should copy the string instead of reassigning the pointer:

strcpy(final[i], token[i]);

---

On an unrelated note, there's no need for token to be an array of pointer. It can be just a pointer:

char *token = strtok(...);

---

Example of a possible implementation:

char **analyze(char *buffer)
{
    size_t current_token_index = 0;
    char **tokens = NULL;

    // Get the first "token"
    char *current_token = strtok(buffer, " ");

    while (current_token != NULL)
    {
        // (Re)allocate memory for the tokens array
        char **temp = realloc(tokens, sizeof *temp * (current_token_index + 1));
        if (temp == NULL)
        {
            // TODO: Better error handling
            //       (like freeing the tokens already allocated)
            return NULL;
        }

        tokens = temp;

        // Allocate memory for the "token" and copy it
        tokens[current_token_index++] = strdup(current_token);

        // Get the next "token"
        current_token = strtok(NULL, " ");
    }

    // Final reallocation to make sure there is a terminating null pointer
    char **temp = realloc(tokens, sizeof *temp * (current_token_index + 1));
    if (temp == NULL)
    {
        // TODO: Better error handling
        //       (like freeing the tokens already allocated)
        return NULL;
    }

    tokens = temp;

    // Terminate the array
    tokens[current_token_index] = NULL;

    return tokens;
}

Note that strdup isn't a standard C function, but it is prevalent enough to assume it will exist. In the unlikely case where it doesn't exist, it's easy to implement yourself.