Why am I getting a heap-use-after-free error?

Question

Why am I getting a segmentation fault? When I compile with sanitize=address I get a heap-use-after-free which I don't quite understand (the reason for). I get heap-use-after-free on address xyz.

Read of size 8 in ... test.c:22 (the line that prints parts[i])

... is located 0 bytes inside of a 8-byte region freed here (strings.c:175, which is the reallocarray line)

... previously allocated here(in main test.c:9, which is the char **parts=calloc.. line)

this example compiles standalone:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
enum EXPLODE_FLAGS {
    NO_FLAGS=0,
    FLAG_TRIM=1, // trim each line of the output
};
typedef enum RESULT {
    E_SUCCESS=0, // not a failure but a SUCCESS
    E_ERROR=1, // failure due to generic error
    E_ARGS=2,   // failed due to arguments
    E_MALLOC=3
} RESULT;
enum EXP_RESULT {
    EXP_ERROR=-E_ERROR, // generic error
    EXP_ARGS=-E_ARGS, // generic error with the arguments
    EXP_MALLOC=-E_MALLOC, // failure due to generic error
    EXP_SEP=-4, // separator is null
    EXP_INPUT=-5, // input is a null pointer
    EXP_OUTPUT=-6, // output is a null pointer
};
int str_explode(char *input, char **parts, const char separator) {
    int partCounter = 0;
    int currentPartLength = 0;
    char *currentPart = NULL;
    // Check for input validity
    if (!input) return EXP_INPUT;
    if (!parts) return EXP_OUTPUT;
    if (separator == '\0') return EXP_SEP;
    char *start = input;
    char *currentPartStart = input;
    char *end = input + strlen(input);
    fprintf(stdout,"Inside the function\n");
    for (char *thischar = start; thischar <= end; thischar++) {
        if (*thischar == separator || *thischar == '\0') {
            printf("Inside check; current char is: %c\n",*thischar);
            // Allocate memory for the length of the current part + null terminator
            currentPart = calloc(1, currentPartLength + 1);
            if (!currentPart) {
                // Use goto for cleanup
                goto cleanup;
            }
            // Copy the current part into the allocated memory
            if (currentPartLength > 0) {
                strncpy(currentPart, currentPartStart, currentPartLength);
                currentPart[currentPartLength] = '\0';  // Null-terminate the string
            } else {
                currentPart[0] = '\0';  // Empty string for the case of consecutive separators
            }
            // Reallocate memory for another char pointer
            parts=reallocarray(parts,partCounter+1,sizeof(char*));
            if (!parts) {
                // Use goto for cleanup
                goto cleanup_current_part;
            }
            printf("About to add current part (%s) to the pile\n",currentPart);
            // Add the new string part
            parts[partCounter++] = currentPart;
            printf("About to check current part from the pile: %s\n",parts[partCounter-1]);
            // Reset variables for the next part
            currentPart = NULL;
            currentPartStart = thischar + 1;  // Skip the separator
            currentPartLength = 0;
            if('\0'==*thischar)
                break;
        } else {
            ++currentPartLength;
        }
    }

    free(currentPart);
    return partCounter;

    // Label for cleanup
cleanup_current_part:
    fprintf(stderr,"Unable to allocate memory for another part\n");
    free(currentPart);
cleanup:
    fprintf(stderr,"Unable to allocate memory for current part\n");
    // Free previously allocated memory before returning error
    for (int i = 0; i < partCounter; i++) {
        free(parts[i]);
    }
    free(parts);

    return EXP_MALLOC;
}

int main(void) {
    char *input = "apple;orange;banana;grape";
    char **parts = calloc(1,1*sizeof(char*));
    parts[0]="\0";
    int partCount = str_explode(input, parts, ';');
    if (partCount < 0) {
        printf("Error code #%d\n", -partCount);
        return 1;
    }

    printf("Original string: %s\n", input);
    printf("Number of parts: %d\n", partCount);
    for (int i = 0; i < partCount; i++) {
        printf("About to print part #%d:\n",i+1);
        printf("Part %d: %s\n", i + 1, parts[i]);
        free(parts[i]);
    }

    free(parts);

    return 0;   
}

Please bear in mind that I am not an experienced C programmer. I have a working knowledge of pointers but I'm just not able to wrap my head around what I'm doing wrong here. The point of this little program is to improve my understanding of working with character arrays in C.

Answer 1

In C arguments are passed by value which means the changes to parts itself are lost (and leaked) upon return of str_explode(). This also means you end up doing a double free of the parts both in str_explore() as part of reallocarray() and the same (unchanged) variable in main().

The minimal fix is to pass in the address of the parts and change the argument to char ***parts and update all uses to be (*parts):

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

enum EXPLODE_FLAGS {
    NO_FLAGS=0,
    FLAG_TRIM=1, // trim each line of the output
};

typedef enum RESULT {
    E_SUCCESS=0, // not a failure but a SUCCESS
    E_ERROR=1, // failure due to generic error
    E_ARGS=2,   // failed due to arguments
    E_MALLOC=3
} RESULT;

enum EXP_RESULT {
    EXP_ERROR=-E_ERROR, // generic error
    EXP_ARGS=-E_ARGS, // generic error with the arguments
    EXP_MALLOC=-E_MALLOC, // failure due to generic error
    EXP_SEP=-4, // separator is null
    EXP_INPUT=-5, // input is a null pointer
    EXP_OUTPUT=-6, // output is a null pointer
};

int str_explode(char *input, char ***parts, const char separator) {
    int partCounter = 0;
    int currentPartLength = 0;
    char *currentPart = NULL;
    // Check for input validity
    if (!input) return EXP_INPUT;
    if (!parts) return EXP_OUTPUT;
    if (separator == '\0') return EXP_SEP;
    char *start = input;
    char *currentPartStart = input;
    char *end = input + strlen(input);
    fprintf(stdout,"Inside the function\n");
    for (char *thischar = start; thischar <= end; thischar++) {
        if (*thischar == separator || *thischar == '\0') {
            printf("Inside check; current char is: %c\n",*thischar);
            // Allocate memory for the length of the current part + null terminator
            currentPart = calloc(1, currentPartLength + 1);
            if (!currentPart) {
                // Use goto for cleanup
                goto cleanup;
            }
            // Copy the current part into the allocated memory
            if (currentPartLength > 0) {
                strncpy(currentPart, currentPartStart, currentPartLength);
                currentPart[currentPartLength] = '\0';  // Null-terminate the string
            } else {
                currentPart[0] = '\0';  // Empty string for the case of consecutive separators
            }
            // Reallocate memory for another char pointer
            (*parts)=reallocarray((*parts),partCounter+1,sizeof(char*));
            if (!(*parts)) {
                // Use goto for cleanup
                goto cleanup_current_part;
            }
            printf("About to add current part (%s) to the pile\n",currentPart);
            // Add the new string part
            (*parts)[partCounter++] = currentPart;
            printf("About to check current part from the pile: %s\n",(*parts)[partCounter-1]);
            // Reset variables for the next part
            currentPart = NULL;
            currentPartStart = thischar + 1;  // Skip the separator
            currentPartLength = 0;
            if('\0'==*thischar)
                break;
        } else {
            ++currentPartLength;
        }
    }

    free(currentPart);
    return partCounter;

    // Label for cleanup
cleanup_current_part:
    fprintf(stderr,"Unable to allocate memory for another part\n");
    free(currentPart);
cleanup:
    fprintf(stderr,"Unable to allocate memory for current part\n");
    // Free previously allocated memory before returning error
    for (int i = 0; i < partCounter; i++) {
        free(*parts[i]);
    }
    free(*parts);

    return EXP_MALLOC;
}

int main(void) {
    char *input = "apple;orange;banana;grape";
    char **parts = calloc(1,1*sizeof(char*));
    parts[0]="\0";
    int partCount = str_explode(input, &parts, ';');
    if (partCount < 0) {
        printf("Error code #%d\n", -partCount);
        return 1;
    }

    printf("Original string: %s\n", input);
    printf("Number of parts: %d\n", partCount);
    for (int i = 0; i < partCount; i++) {
        printf("About to print part #%d:\n",i+1);
        printf("Part %d: %s\n", i + 1, parts[i]);
        free(parts[i]);
    }

    free(parts);

    return 0;   
}

and the resulting output:

Inside the function
Inside check; current char is: ;
About to add current part (apple) to the pile
About to check current part from the pile: apple
Inside check; current char is: ;
About to add current part (orange) to the pile
About to check current part from the pile: orange
Inside check; current char is: ;
About to add current part (banana) to the pile
About to check current part from the pile: banana
Inside check; current char is:
About to add current part (grape) to the pile
About to check current part from the pile: grape
Original string: apple;orange;banana;grape
Number of parts: 4
About to print part #1:
Part 1: apple
About to print part #2:
Part 2: orange
About to print part #3:
Part 3: banana
About to print part #4:
Part 4: grape

This will get you labeled as a 3-star programmer. Two good redesigns would be to return the parts and use a sentinel NULL to signify the there no more elements. Or create a struct to hold your two result values:

struct result {
   char **parts;
   int partCount;
}

which you can either return or use an an out parameter.

I suggest you initialize char **parts = NULL and let str_explode() allocate however many values you need. It's defect to free a constant parts[0] = "\0"; ... free(parts[i]). As you change it anyways, don't initialize it in the first place.

"\0" means { '\0', '\0' } so I suggest either '\0' or "" (but see above).

parts=reallocarray(parts, ...) leaks the original parts if reallocarray() fails. Assign the result to temporary.

Answer 2

There are at least two errors in your shown code (I have not checked it thoroughly)

At least in:

            currentPart = calloc(1, currentPartLength + 1);
            if (!currentPart) {
                // Use goto for cleanup
                goto cleanup;
            }

and then in:

cleanup:
    fprintf(stderr,"Unable to allocate memory for current part\n");
    // Free previously allocated memory before returning error
    for (int i = 0; i < partCounter; i++) {
        free(*parts[i]);
    }
    free(*parts);

This code is, for sure incorrect, as *parts[i] is, for sure not what you want (as it is interpreted as *(parts[i]) and not as (*parts)[i], by the way you end calling free(*parts).

You wanted a reference pointer to an array of pointers to char arrays, and you pass a pointer reference to a calling code's variable, which is something declared correctly in your code as:

    char ***parts,

and initialize it with an array acquired with calloc (which you have allocated already, and the allocated memory in main is lost in the assignments you make with the new allocations you make in the function, leaking the memory allocated in main()).

(Comment: You should not write such large functions like str_explode() They are very difficult to manage and maintain. Also, using three level pointers is error prone.)

In relation with the errors, I think you should have

    free((*parts)[i]); /* this way you free the pointer at position
                        * i in the array pointed to by *parts, and
                        * don't access i positions after the
                        * referenced pointer and call free on an
                        * invalid pointer --reinterpreted from what
                        * is found there */

instead (but this is not the error you claim about, this is a different error you also have in the code). This will use the pointer you have initialized allocating an array of pointers to char, with the pointer stored by *parts.

Another problem (and finally, this is the one you ask about) is that you just call free() to free the memory pointed by the pointers, but this don't reassign NULL to the pointers themshelves. Free() is passed a pointer as value (not by reference), so free() has no means of making NULL the variable holding that pointer you passed (indeed free() doesn't know --neither needs to-- that you pass a value that is stored in a variable). So if you free(*parts);, then you have also to do:

    free(*parts);
    *parts = NULL;

and this will make your calling code to know that nothing has been allocated as result, because (if you don't) your calling code will see a non-null pointer, and will start accessing and derreferencing it (and this is the error you claim you are being told).

You could also do

    for (int i = 0; i < partCounter; i++) {
        free((*parts)[i]);
        (*parts)[i] = NULL;
    }

but as you are going to free(*parts) and the block is not going to be accessible anymore, after you return it to the heap, this last step is not necessary.

You also have issues related to idioms that you are wrongly applying for a I don't know why, but I've been told to with no reason (e.g. null terminating a string that you have copied with strncpy()) but I leave other answers to make you focus on these.