Why does free() cause an error in my code when it's there, but everything runs well when it is not there?

user_8902744 0
Viewed 134

My program takes an arbitrary number of words at runtime and stores them in a dynamically-sized array of words.

Currently, my program runs well, except when I use free() to free up the memory of the temporary double pointer temp. I am not quite sure why it does this, as I thought it would cause errors if I didn't use it.

int wordSize = 10, arrSize = 1, i = 0;
char **stringArr, **temp;
char *input;

stringArr = malloc(arrSize * sizeof(char *));

puts("Accepting input...");

for (;;) {
    if (i >= arrSize) {
        arrSize += 1;
        temp = realloc(stringArr, arrSize * sizeof(char *));

        if (temp != NULL) {
            stringArr = temp;
            free(temp); // This is the line that is giving me issues; removing it works
        } else {
            puts("Could not allocate more memory");
            return 0;
        }
    }

    stringArr[i] = malloc(sizeof(input));
    input = malloc(wordSize * sizeof(char));
    scanf("%10s", input);

    if (strcmp(input, "END")) {
        strcpy(stringArr[i], input);
        i++;
    } else
        break;
       
}
free(stringArr);

At the bottom of my program I use free() without any issues. How come it works OK here but not earlier on in the program.

I feel I am missing something about how free() works.

Note: this is my first program implementing malloc() and realloc(), so I am only just getting used to how they work. If you know of a better way to accomplish what I am doing that, please feel free to describe.

realloc dynamic-memory-allocation malloc memory-management c
2 Answers

The free(temp); line is causing an error (later on) because, in the preceding line, stringArr = temp;, you are assigning the address that is stored in the temp pointer to that in the stringArr pointer. Thus, when you free the memory pointed to by temp you also free the memory pointed to by stringArr, because it is the same memory block. Copying a pointer's value from one variable to another does not make a (separate) copy of the memory.

Omitting the free(temp); line is correct, because that memory is freed later on, in the free(stringArr); call.

user_108710730

You must not free the reallocated array when reallocation was successful. If you do that, the code will modify this freed block, which has undefined behavior and you will have further undefined behavior when you later try and reallocate or free this block.

Note also the following:

  • pre-allocating stringArr with a size of 1 is not necessary. Just initialize stringArr to 0 and arrSize to 0. realloc() can take a null pointer and will behave like malloc().

  • stringArr[i] = malloc(sizeof(input)); is incorrect: it will allocate a char array with a size of 4 or 8 depending on the size of a pointer on the target architecture, not 11 bytes as it should.

  • if the wordSize is the maximum length of a word, you should allocate one more byte for the null terminator. The 10 in %10s must match the value of wordSize, which is cumbersome because there is no easy way to pass this to scanf() as a variable.

  • you do not check the return value of scanf(), causing undefined behavior in case of premature end of file.

  • you have memory leaks: input is allocated for each iteration but never freed, freeing stringArr without freeing the strings pointed to by its elements makes them inaccessible.

It would be more efficient to use a local array to try and read the words with scanf() and only allocate the string and reallocate the array if successful.

Here is a modified version:

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

int main() {
    int arrSize = 0;
    char **stringArr = NULL;
    char input[11];
    
    puts("Accepting input...");
    
    while (scanf("%10s", input) == 1 && strcmp(input, "END") != 0) {
        char **temp = realloc(stringArr, (arrSize + 1) * sizeof(*stringArr));
        if (temp != NULL) {
            stringArr = temp;
        } else {
            puts("Could not allocate more memory");
            break;
        }
        stringArr[arrSize] = strdup(input);
        if (stringArr[arrSize] == NULL) {
            puts("Could not allocate more memory");
            break;
        }
        arrSize++;
    }
    puts("Array contents:");
    for (int i = 0; i < arrSize; i++) {
        printf("%i: %s\n", i, stringArr[i]);
    }
    for (int i = 0; i < arrSize; i++) {
        free(stringArr[i]);
    }
    free(stringArr);
    return 0;
}
user_45932670
Related