I wrote a program that uses structures and dynamic memory allocation. I would like to get help understanding why if I press 4 (which means exit the menu) I get the error:
HEAP CORRUPTION DETECTED: after Normal block (#76) at 0x00845EE8.
It is linked to the function deletelist()
. I really don't understand why it happens and how to fix this problem.
This is the code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define PRO_OP 1
#define CON_OP 2
#define PRINT_OP 3
#define EXIT_OP 4
#define STR_LEN 50
#define MAX_LIST_LENGTH 10
typedef struct reasonList
{
char* listName;
char* reasons[MAX_LIST_LENGTH];
int numReasons;
} reasonList;
void initList(reasonList* list, char* name);
void addReason(reasonList* list);
void printList(reasonList list);
int menu(void);
void myFgets(char str[], int n);
void deleteList(reasonList* list);
int main(void)
{
char dilemma[STR_LEN] = { 0 };
int op = 0;
reasonList proList;
initList(&proList, "PRO");
reasonList conList;
initList(&conList, "CON");
printf("What is your dilemma?\n");
myFgets(dilemma, STR_LEN);
while (op != EXIT_OP)
{
op = menu();
switch (op)
{
case(PRO_OP):
addReason(&proList);
break;
case(CON_OP):
addReason(&conList);
break;
case(PRINT_OP):
printf("Your dilemma:\n");
printf("%s\n\n", dilemma);
printList(proList);
printList(conList);
break;
case(EXIT_OP):
deleteList(&proList);
deleteList(&conList);
break;
}
}
printf("Good luck!\n");
getchar();
return 0;
}
/*
Function will initialize a reason list
input: the list to init, and its name
output: none
*/
void initList(reasonList* list, char* listName)
{
list->listName = (char*) malloc(sizeof(char));
strcpy(list->listName, listName);//equal to PRO or CON
for (int i = 0; i < MAX_LIST_LENGTH; i++)
{
list->reasons[i] = (char*)malloc(sizeof(char) * STR_LEN);
if (list->reasons[i] == NULL)
{
printf("no memmory left\n");
exit(1);
}
strcpy(list->reasons[i], "");
}
list->numReasons = 0;
}
/*
Function will add a reason to the list
input: the list to add to and its name
output: none
*/
//
void addReason(reasonList* list)
{
char* newReason = (char*)malloc(sizeof(char) * STR_LEN);
printf("Enter a reason to add to the list %s:\n", list->listName);
myFgets(newReason, STR_LEN);
if (list->numReasons >= MAX_LIST_LENGTH)//if no memory left
{
//free(newReason);
return;
}
free(list->reasons[list->numReasons]);
list->reasons[list->numReasons] = newReason;
list->numReasons++;
list->reasons[list->numReasons] = NULL;
}
/*
Function will print a list of reasons
input: the list
output: none
*/
void printList(reasonList list)
{
printf("list %s\n--------\n", list.listName);
for (int i = 0; i < list.numReasons; i++)
{
printf("%s", list.reasons[i]);
printf("\n");
}
printf("\n");
}
/*
Function shows menu and returns user's choice
input: none
output: user's choice
*/
int menu(void)
{
int op = 0;
printf("Choose option:\n");
printf("%d - Add PRO reason\n", PRO_OP);
printf("%d - Add CON reason\n", CON_OP);
printf("%d - Print reasons\n", PRINT_OP);
printf("%d - Exit\n", EXIT_OP);
scanf("%d", &op);
while (op < PRO_OP || op > EXIT_OP)
{
printf("Invalid option. Try again: ");
scanf("%d", &op);
}
getchar(); // clean buffer
return op;
}
/*
Function will delete a list
input: the list to delete
output: none
*/
void deleteList(reasonList* list)
{
for (int i = 0; i < list->numReasons; i++)
{
free(list->reasons[i]);
list->reasons[i] = NULL; // set pointer to NULL
}
free(list->listName);
list->listName = NULL; // set pointer to NULL
}
/*
Function will perform the fgets command and also remove the newline
that might be at the end of the string - a known issue with fgets.
input: the buffer to read into, the number of chars to read
*/
void myFgets(char str[], int n)
{
fgets(str, n, stdin);
str[strcspn(str, "\n")] = 0;
}
I was not able to reproduce the segfault but valgrind complained about
initList()
: malloc(sizeof(char))
which allocates 1 bytes (the \0
for an empty string) while you need strlen(listName) + 1
.
Unrelated, to eliminate the memory leak, observe that you in initList()
allocate space for MAX_LIST_LENGTH
reasons
but in deleteList()
you only free list->numReasons
. Suggest you just leave it to addReasons()
to allocate space:
void initList(reasonList* list, char* listName) {
list->listName = malloc(strlen(listName) + 1);
strcpy(list->listName, listName);
list->numReasons = 0;
}
void addReason(reasonList* list) {
if (list->numReasons >= MAX_LIST_LENGTH)
return;
list->reasons[list->numReasons] = malloc(STR_LEN);
printf("Enter a reason to add to the list %s:\n", list->listName);
myFgets(list->reasons[list->numReasons++], STR_LEN);
}
If you want to be able to delete (and reuse) reasons[i]
then in initList()
you want to initialize list->reasons[i] = NULL
and I suggest (for symmetry) create a deleteReason()
function which you then use in deleteList()
.
Another valid design (as your reasons
are fixed size) is to allocate all the reasons
in initList()
(like you had). In addReaon()
reuse the allocated string (no malloc()
or free()
). In deleteList()
free all MAX_LIST_LENGTH
reasons.
It's a good idea to check the return value from malloc()
.
Don't cast the void *
from malloc()
.
Prefer passing a variable opposed to a type to sizeof
. In the above instead of sizeof(char)
use sizeof *list->listName
or sizeof *listName; that said
size(char)is defined as
1` so I always leave it out.