Heap corruption error while trying to free a two-dimentional array

Question

My program receives a two-dimentional array of strings. After it does what it should do i try to free the memory allocated for the strings but i get a Heap corruption error when i try to do so. Here's my code:

printf("Please enter the number of lines:");
        scanf("%d", &length);
        lines = (char**)malloc(length*sizeof(char*));//allocating memory for the array of strings "lines".
        if (lines == NULL)
        {
            printf("Error! Not enough memory.");
            return 0;
        }
        getchar();//needed in order to use "gets".
        printf("Please enter your lines:\n");
        for (i = 0; i < length; i++)
        {
            printf("%d.", i);
            gets(buffer);//user inserts the string "buffer".
            lines[i] = (char*)malloc(strlen(buffer)*sizeof(char));//allocating memory for the strings in the array according to the ength of the string "buffer".
            if (lines[i] == NULL)
            {
                printf("Error! Not enogh memory.");
                return 0;
            }
            strcpy(lines[i], buffer);//copies the string "buffer" into "lines[i]".
        }
        printf("Plese enter your comparison string:");
        text = (char*)malloc(80 * sizeof(char) + 1);//allocating temporary memory for the string "text".
        gets(text);//inserting the string "text".
        text = (char*)realloc(text, strlen(text)*sizeof(char)+1);//reallocating the correct memory of "text" according to its exact length and adding an additional memory spot for the chat "\0".
        cmpresult = isEquivalent(lines, length, text);//calling the function
        if (cmpresult == TRUE)//if the function returned TRUE
            printf("The strings are equal.\n\n");
        else printf("The strings are not equal.\n\n");//if the function returned FALSE.
        for ( i = 0; i < length; i++)//free the memory allocated for lines[i].
            free(lines[i]);
        free(lines);//free the memory allocated for the array "lines".
        getchar(option);
    }

After debbuging, i found oout that this is the line that crashes my program.

for (i=0; i < length; i++)//free the memory allocated for lines[i].
 free(lines[i]);

I'm due in a day and I'm desperate! Thank you.

Should you not allocate one byte for the null terminator in your second call to malloc? — Shadowwolf, Dec 29 '15 at 18:52
`malloc(strlen(buffer)*sizeof(char));` --> `malloc(strlen(buffer)+1);`. C strings are 0-terminated. (And simplified because `sizeof(char)` is`1` by definition.) — Weather Vane, Dec 29 '15 at 18:53
You also should not cast the return type of `malloc` in C, this can hide errors if you forget to include its header. — Shadowwolf, Dec 29 '15 at 18:57
Look at the three lines starting `text = ...`. First, `text `is not declared. Second, `gets` is deprecated, third, it's useless trying to re-allocate `text` on the basis of what you already entered. The input has already broken the buffer at that point. — Weather Vane, Dec 29 '15 at 19:01
@WeatherVane I don't understand why you didn't _answer_ the question. — Motomotes, Dec 29 '15 at 19:35
@Motes I didn't know it would completely solve it; I thought I was looking at a "can of worms" because the code posted is incomplete (see my subsequent comment that nobody acknowledged). — Weather Vane, Dec 29 '15 at 20:44
@Shadowwolf Please explain me something, Which kind of compiler this days does compile a program which doesn't have `#include `, when you call `malloc`? — Michi, Dec 29 '15 at 22:20
Trying to compile a program like [This](http://ideone.com/lBeUt1) with a modern compiler like `GCC` you'll get [This](http://pastebin.com/raw/k6WZKJib). Except when you use IDEONE, which should not be used as example — Michi, Dec 29 '15 at 22:28
In C, implicit declarations are allowed. The implicitly declared function will return `int` and take a fixed number of arguments (as provided). You can cast the returned `int` to a pointer type in C. Most compilers will warn you about the implicit declaration though, but this is not required. When you omit the cast, your compiler will have to output a warning since you are then trying to assign an `int` to a pointer without a cast, which is forbidden. Therefor omitting the cast can prevent possible errors in C. — Shadowwolf, Dec 30 '15 at 11:38

score 1 · Answer 1 · edited May 23 '17 at 10:28

1

There are several problems:

You don't allocate enough memory for strings. As Weather Vane said, C string are zero-terminated, you need to take account of that '0' and allocate one more byte each time you allocate place for a character string.
Don't use strcpy() but strncpy() instead. This is for security issues. See Why should you use strncpy instead of strcpy?
For the same reasons, don't use gets()
text is not freed (that's your original problem)

edited May 23 '17 at 10:28

Community

1
1

answered Dec 29 '15 at 20:15

Pierre

1,162
12
29

`strncpy` is worse than `strcpy` imo, because sometimes it produces a non-null-terminated string. It is correct and less error-prone to allocate the right amount of space and then use `strcpy` – M.M Dec 29 '15 at 21:01

score 1 · Answer 2 · answered Dec 29 '15 at 20:30

Overflow happens here for each line:

gets(buffer);
lines[i] = (char*)malloc(strlen(buffer)*sizeof(char));
if (lines[i] == NULL)
{
    printf("Error! Not enogh memory.");
    return 0;
}
strcpy(lines[i], buffer); // <----- overflow happens here

To fix this change:

lines[i] = (char*)malloc(strlen(buffer)*sizeof(char));

to:

lines[i] = (char*)malloc((strlen(buffer)+1)*sizeof(char));

The realloc() in the following code does not help with the overflow. By that time overflow has already happened (if it happens).

text = (char*)malloc(80 * sizeof(char) + 1);
gets(text); // <----- overflow happens here if string > 80
text = (char*)realloc(text, strlen(text)*sizeof(char)+1);

Heap corruption error while trying to free a two-dimentional array

2 Answers2