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.
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.