"Pointer being freed was not allocated" happen on mac but not on window7

Question

I am doing an exercise on a book, changing the words in a sentence into pig latin. The code works fine in window 7, but when I compiled it in mac, the error comes out.

After some testings, the error comes from there. I don't understand the reason of this problem. I am using dynamic memories for all the pointers and I have also added the checking of null pointer.

while (walker != NULL && *walker != NULL){
    free(**walker); 
    free(*walker);
    free(walker); 

    walker++;
}

Full source code:

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

#define inputSize 81
void getSentence(char sentence [], int size);
int countWord(char sentence[]);
char ***parseSentence(char sentence[], int *count);
char *translate(char *world);
char *translateSentence(char ***words, int count);

int main(void){
    /* Local definition*/
    char sentence[inputSize];
    int wordsCnt;
    char ***head;
    char *result;

    getSentence(sentence, inputSize);
    head = parseSentence(sentence, &wordsCnt);

    result = translateSentence(head, wordsCnt);
    printf("\nFinish the translation: \n");
    printf("%s", result);


    return 0;
}

void getSentence(char sentence [81], int size){
    char *input = (char *)malloc(size);
    int length;

    printf("Input the sentence to big latin : ");
    fflush(stdout);
    fgets(input, size, stdin);

    // do not copy the return character at inedx of length - 1
    // add back delimater 
    length = strlen(input);
    strncpy(sentence, input, length-1);
    sentence[length-1]='\0';

    free(input);
}

int countWord(char sentence[]){
    int count=0;

    /*Copy string for counting */
    int length = strlen(sentence);
    char *temp = (char *)malloc(length+1);
    strcpy(temp, sentence);

    /* Counting */
    char *pToken = strtok(temp, " ");
    char *last = NULL;
    assert(pToken == temp);
    while (pToken){
        count++;

        pToken = strtok(NULL, " ");
    }

    free(temp);
    return count;
}
char ***parseSentence(char sentence[], int *count){
    // parse the sentence into string tokens
    // save string tokens as a array
    // and assign the first one element to the head
    char *pToken;
    char ***words;
    char *pW;

    int noWords = countWord(sentence);
    *count = noWords;

    /* Initiaze array */
    int i;
    words = (char ***)calloc(noWords+1, sizeof(char **));
    for (i = 0; i< noWords; i++){
        words[i] = (char **)malloc(sizeof(char *));
    }

    /* Parse string */
    // first element
    pToken = strtok(sentence, " ");

    if (pToken){
        pW = (char *)malloc(strlen(pToken)+1);
        strcpy(pW, pToken);
        **words = pW;
        /***words = pToken;*/

        // other elements
        for (i=1; i<noWords; i++){
            pToken = strtok(NULL, " ");
            pW = (char *)malloc(strlen(pToken)+1);
            strcpy(pW, pToken);
            **(words + i) = pW;
            /***(words + i) = pToken;*/
        }
    }

    /* Loop control */
    words[noWords] = NULL;


    return words;
}

/* Translate a world into big latin */
char *translate(char *word){
    int length = strlen(word);
    char *bigLatin = (char *)malloc(length+3);

    /* translate the word into pig latin */
    static char *vowel = "AEIOUaeiou";
    char *matchLetter;
    matchLetter = strchr(vowel, *word);
    // consonant
    if (matchLetter == NULL){
        // copy the letter except the head
        // length = lenght of string without delimiter
        // cat the head and add ay
        // this will copy the delimater,
        strncpy(bigLatin, word+1, length);
        strncat(bigLatin, word, 1);
        strcat(bigLatin, "ay");
    }
    // vowel
    else {
        // just append "ay"
        strcpy(bigLatin, word);
        strcat(bigLatin, "ay");
    }


    return bigLatin;
}

char *translateSentence(char ***words, int count){
    char *bigLatinSentence;
    int length = 0;
    char *bigLatinWord;

    /* calculate the sum of the length of the words */
    char ***walker = words;
    while (*walker){
        length += strlen(**walker);
        walker++;
    }

    /* allocate space for return string */
    // one space between 2 words
    // numbers of space required = 
    // length of words
    // + (no. of words * of a spaces (1) -1 ) 
    // + delimater
    // + (no. of words * ay (2) )
    int lengthOfResult = length + count + (count * 2);
    bigLatinSentence = (char *)malloc(lengthOfResult);
    // trick to initialize the first memory 
    strcpy(bigLatinSentence, "");

    /* Translate each word */
    int i;
    char *w;
    for (i=0; i<count; i++){
        w = translate(**(words + i));
        strcat(bigLatinSentence, w);
        strcat(bigLatinSentence, " ");
        assert(w != **(words + i));
        free(w);
    }


    /* free memory of big latin words */
    walker = words;
    while (walker != NULL && *walker != NULL){
        free(**walker); 
        free(*walker);
        free(walker); 

        walker++;
    }

    return bigLatinSentence;
}

Answer 1

Your code is unnecessarily complicated, because you have set things up such that:

• n: the number of words
• words: points to allocated memory that can hold n+1 char ** values in sequence
• words[i] (0 <= i && i < n): points to allocated memory that can hold one char * in sequence
• words[n]: NULL
• words[i][0]: points to allocated memory for a word (as before, 0 <= i < n)

Since each words[i] points to stuff-in-sequence, there is a words[i][j] for some valid integer j ... but the allowed value for j is always 0, as there is only one char * malloc()ed there. So you could eliminate this level of indirection entirely, and just have char **words.

That's not the problem, though. The freeing loop starts with walker identical to words, so it first attempts to free words[0][0] (which is fine and works), then attempts to free words[0] (which is fine and works), then attempts to free words (which is fine and works but means you can no longer access any other words[i] for any value of i—i.e., a "storage leak"). Then it increments walker, making it more or less equivalent to &words[1]; but words has already been free()d.

Instead of using walker here, I'd use a loop with some integer i:

for (i = 0; words[i] != NULL; i++) {
    free(words[i][0]);
    free(words[i]);
}
free(words);

I'd also recommending removing all the casts on malloc() and calloc() return values. If you get compiler warnings after doing this, they usually mean one of two things:

• you've forgotten to #include <stdlib.h>, or
• you're invoking a C++ compiler on your C code.

The latter sometimes works but is a recipe for misery: good C code is bad C++ code and good C++ code is not C code. :-)

---

Edit: PS: I missed the off-by-one lengthOfResult that @David RF caught.

Answer 2

int lengthOfResult = length + count + (count * 2);

must be

int lengthOfResult = length + count + (count * 2) + 1; /* + 1 for final '\0' */

---

while (walker != NULL && *walker != NULL){
    free(**walker); 
    free(*walker);
    /* free(walker); Don't do this, you still need walker */
    walker++;
}
free(words); /* Now */

---

And you have a leak:

int main(void)
{
    ...
    free(result); /* You have to free the return of translateSentence() */
    return 0;
}

Answer 3

In this code:

while (walker != NULL && *walker != NULL){
    free(**walker); 
    free(*walker);
    free(walker); 

    walker++;
}

You need to check that **walker is not NULL before freeing it.

Also - when you compute the length of memory you need to return the string, you are one byte short because you copy each word PLUS A SPACE (including a space after the last word) PLUS THE TERMINATING \0. In other words, when you copy your result into the bigLatinSentence, you will overwrite some memory that isn't yours. Sometimes you get away with that, and sometimes you don't...

Answer 4

Wow, so I was intrigued by this, and it took me a while to figure out.

Now that I figured it out, I feel dumb.

What I noticed from running under gdb is that the thing failed on the second run through the loop on the line

free(walker);

Now why would that be so. This is where I feel dumb for not seeing it right away. When you run that line, the first time, the whole array of char*** pointers at words (aka walker on the first run through) on the second run through, when your run that line, you're trying to free already freed memory.

So it should be:

while (walker != NULL && *walker != NULL){
    free(**walker); 
    free(*walker);

    walker++;
}

free(words); 

Edit:

I also want to note that you don't have to cast from void * in C.

So when you call malloc, you don't need the (char *) in there.