free(): invalid pointer when freeing a 2d pointer

user_10756677 0
Viewed 194

I have a 2d pointer array:

char **fields = calloc(1, sizeof(char *));

I add to it different strings, like this:

if(i > 0) fields = realloc(fields, (i+1) * sizeof(char *));
fields[i] = calloc(size, sizeof(char));

I then use memcpy into the fields[i] the desired string.

At the end of the program, when I try to free fields, I do it like this:

int j=0
while(fields != NULL && fields[j]){
    free(fields[j]);
    j++;
}
free(fields);

The program inserts 4 strings into fields. The first string frees as expected, however on the second iteration of the loop (j=1) the program stops and outputs the error: free(): invalid pointer

EDIT: I made a short program with the same problem:

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

int main(int argc, char *argv[]){
    char **fields = calloc(1, sizeof(char *));
    int fieldsIndex = 0,i=0;
    while (i<4) {
        if(fieldsIndex > 0){
            fields = realloc(fields, (fieldsIndex + 1) * sizeof(char *));
            fields[fieldsIndex] =NULL;
            printf("amount of field places: %d\n", (fieldsIndex + 1));
        }

        fields[fieldsIndex] = calloc(8, sizeof(char));
        fields[fieldsIndex] = "88888888";
        fieldsIndex++;
        i++;
    }
    int j=0;
    for(j=0; j<i; j++){
        printf("field: %s\n", fields[j]);
        free(fields[j]);
    }
    free(fields);
    return 0;
}

Can anyone help?

free pointers c
1 Answers

Addressing mainly the MRE.

  • The main problems are around this line:

    fields[fieldsIndex] = "88888888";

    It's not right for two reasons:

    • Firstly you need one more element in the array for the null byte.

    • Secondly, you make the fields[fieldsIndex] pointers point to string literals, it not only causes a memory leak, but also those string literals are usually stored in a readonly section of memory, either way the behavior freeing a pointer pointing to a string literal is undefined.

      You need to copy the strings to the memory you just allocated. Using memcpy should work as long as you reserve enough memory as mentioned in the previous point, a cleaner way would be to use strdup.

  • Another issue is if(fieldsIndex > 0) because then fields[0] will not have allocated memory.

Some other notes, if you know the amount of strings (i < 4) you shouldn't need to realloc, just allocate space for all the pointers in the first calloc* (assuming that is not brought about by the construction of the MRE) , also i and fieldsIndex seem to be redundant.

Here is a demo keeping realloc (as it's tangential to the OP):

int main()
{
    char **fields = NULL;
    char **tempfields; // I advise the use of an auxiliary pointer for reallocation
    int fieldsIndex = 0;

    while (fieldsIndex < 4)
    {
        tempfields = realloc(fields, (fieldsIndex + 1) * sizeof *fields); //*
        if (!tempfields)
        {         
            // handle the allocation error appropriately
        }
        fields = tempfields;
        printf("amount of field places: %d\n", (fieldsIndex + 1));
        fields[fieldsIndex] = strdup("88888888");
        // Or 
        // fields[fieldsIndex] = calloc(9, sizeof **fields); // check return
        // strcpy(fields[fieldsIndex], "88888888");

        fieldsIndex++;
    }

    // With int iterator
    int j = 0;
    for (j = 0; j < fieldsIndex; j++)
    {
        printf("field: %s\n", fields[j]);
        free(fields[j]);
    }
    free(fields);
}

Or with a sentinel element in fields:

Live demo

// With sentinel
tempfields = realloc(fields, (fieldsIndex + 1) * sizeof *fields);
if (!tempfields)
{
     // handle the allocation error appropriately
}
fields = tempfields;
fields[fieldsIndex] = NULL;

while (*fields)
{
    printf("field: %s\n", *fields);
    free(*fields);
    fields++;
}
free(tempfields);
user_68659320
Related