How to find and remove memory leaks and errors in my C program after checking with Valgrind

Question

After checking my code with Valgrind, it shows a lot of error messages and information, and I'm not sure how to find and remove errors in the code?

My source code is as follows.

myheader.h

#include<stdlib.h> #define MAX 15 typedef enum { METROPOLITAN_AREA, TOURIST_AREA, }area_type_t; typedef struct { int population; int area; }metropolitan_t; typedef struct { char *type_place; char *near_airport; }tourist_t; typedef struct { char *name; char *country; area_type_t area_t; union { metropolitan_t metro; tourist_t tourist; }u; }*place_t; extern void get_input(place_t); 

getinput.c

 #include<myheader.h> void get_input(place_t place) { int check=1; int num,i,ch; printf("\nEnter the no of records : \n"); scanf("%d",&num); place=(place_t)malloc(sizeof(place_t)*num); if(NULL==place) { printf("\nMemory allocation failed\n"); exit(1); } for(i=0;i<num;i++) { printf("\nEnter the place name : "); place->name=(char *)malloc(sizeof(char)*MAX); if(NULL==place->name) { free(place); printf("\nMemory allocation failed\n"); exit(1); } scanf("%s",place->name); printf("\nEnter the coutry name : "); place->country=(char *)malloc(sizeof(char)*MAX); if(NULL==place->country) { free(place); free(place->name); printf("Memory allocation failed\n"); exit(1); } scanf("%s",place->country); printf("\nPlease Enter '0' if the place is Metropolitan, '1' if the place Tourist place\n"); scanf("%d",&ch); while(check) { switch(ch) { case 0: check=0; place->area_t=METROPOLITAN_AREA; break; case 1: check=0; place->area_t=TOURIST_AREA; break; default: printf("\nPlease enter valid choice \n"); } } if(place->area_t==METROPOLITAN_AREA) { printf("\nEnter the population of the place : "); scanf("%d",&(place->u.metro.population)); printf("\nEnter the area of place :"); scanf("%d",&(place->u.metro.area)); } else { printf("\nEnter the nearest airport name : "); place->u.tourist.near_airport=(char *)malloc(sizeof(char)*MAX); if(NULL==place->u.tourist.near_airport) { printf("Memory allocation failed\n"); exit(1); } scanf("%s",(place->u.tourist.near_airport)); printf("\nEnter the type of tourist spot : "); place->u.tourist.type_place=(char *)malloc(sizeof(char)*MAX); if(NULL==place->u.tourist.type_place) { printf("Memory allocation failed\n"); exit(1); } scanf("%s",(place->u.tourist.type_place)); } check=1; place++; } } 

main.c

 #include"myheader.h" int main() { place_t place; get_input(place); return 0; } 

Command line transcript

  bash-3.00$ valgrind --tool=memcheck --leak-check=yes --show-reachable=yes a.out ==13514== Memcheck, a memory error detector. ==13514== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al. ==13514== Using LibVEX rev 1575, a library for dynamic binary translation. ==13514== Copyright (C) 2004-2005, and GNU GPL'd, by OpenWorks LLP. ==13514== Using valgrind-3.1.1, a dynamic binary instrumentation framework. ==13514== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al. ==13514== For more details, rerun with: -v ==13514== Enter the no of records : 2 Enter the place name : Banglore Enter the coutry name : India Please Enter '0' if the place is Metropolitan, '1' if the place Tourist place 0 ==13514== Invalid write of size 4 ==13514== at 0x400789: get_input (getinput.c:43) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D040 is 0 bytes after a block of size 16 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x400637: get_input (getinput.c:8) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 4 ==13514== at 0x4007BB: get_input (getinput.c:53) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D040 is 0 bytes after a block of size 16 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x400637: get_input (getinput.c:8) ==13514== by 0x4005E8: main (main.c:5) Enter the population of the place : 303030 Enter the area of place :2500 ==13514== ==13514== Invalid write of size 8 ==13514== at 0x40068C: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D058 is not stack'd, malloc'd or (recently) free'd ==13514== ==13514== Invalid read of size 8 ==13514== at 0x400693: get_input (getinput.c:18) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D058 is not stack'd, malloc'd or (recently) free'd ==13514== ==13514== Invalid read of size 8 ==13514== at 0x4006BF: get_input (getinput.c:24) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D058 is not stack'd, malloc'd or (recently) free'd Enter the place name : London ==13514== ==13514== Invalid write of size 8 ==13514== at 0x4006EE: get_input (getinput.c:26) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D060 is 16 bytes before a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 8 ==13514== at 0x4006F6: get_input (getinput.c:27) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 8 ==13514== at 0x4006F6: get_input (getinput.c:27) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D060 is 16 bytes before a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 8 ==13514== at 0x40072F: get_input (getinput.c:34) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D060 is 16 bytes before a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) Enter the coutry name : London Please Enter '0' if the place is Metropolitan, '1' if the place Tourist place 1 ==13514== ==13514== Invalid write of size 4 ==13514== at 0x40079D: get_input (getinput.c:47) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D068 is 8 bytes before a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid write of size 8 ==13514== at 0x40082F: get_input (getinput.c:62) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D078 is 8 bytes inside a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 8 ==13514== at 0x400837: get_input (getinput.c:63) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D078 is 8 bytes inside a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== Invalid read of size 8 ==13514== at 0x40085B: get_input (getinput.c:68) ==13514== by 0x4005E8: main (main.c:5) ==13514== Address 0x4A2D078 is 8 bytes inside a block of size 15 alloc'd ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) Enter the nearest airport name : London Enter the type of tourist spot : Entertainment ==13514== ==13514== ERROR SUMMARY: 13 errors from 12 contexts (suppressed: 4 from 1) ==13514== malloc/free: in use at exit: 106 bytes in 7 blocks. ==13514== malloc/free: 7 allocs, 0 frees, 106 bytes allocated. ==13514== For counts of detected errors, rerun with: -v ==13514== searching for pointers to 7 not-freed blocks. ==13514== checked 75,768 bytes. ==13514== ==13514== ==13514== 15 bytes in 1 blocks are indirectly lost in loss record 1 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40088A: get_input (getinput.c:70) ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40088A: get_input (getinput.c:70) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== 15 bytes in 1 blocks are definitely lost in loss record 2 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40082E: get_input (getinput.c:62) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== 15 bytes in 1 blocks are definitely lost in loss record 3 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x4006ED: get_input (getinput.c:26) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== 15 bytes in 1 blocks are definitely lost in loss record 4 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== 15 bytes in 1 blocks are indirectly lost in loss record 5 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x4006ED: get_input (getinput.c:26) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== 15 bytes in 1 blocks are indirectly lost in loss record 6 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x40068B: get_input (getinput.c:17) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== ==13514== ==13514== 61 (16 direct, 45 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 7 ==13514== at 0x4904A06: malloc (vg_replace_malloc.c:149) ==13514== by 0x400637: get_input (getinput.c:8) ==13514== by 0x4005E8: main (main.c:5) ==13514== ==13514== LEAK SUMMARY: ==13514== definitely lost: 61 bytes in 4 blocks. ==13514== indirectly lost: 45 bytes in 3 blocks. ==13514== possibly lost: 0 bytes in 0 blocks. ==13514== still reachable: 0 bytes in 0 blocks. ==13514== suppressed: 0 bytes in 0 blocks. 

How can I solve this problem and make my program more memory efficient? Is there something wrong with the malloc() statements I used?

Answer 1

The first thing to do is to get rid of all the memory error (buffer overflow). The first error occurs in get_input.c on line 45, so go to that line and find out what is going wrong.

One thing that is undoubtedly wrong is

  place=(place_t)malloc(sizeof(place_t)*num); 

You allocate place_t, however you defined place_t like this:

 typedef struct { }*place_t; 

i.e. place_t is just a pointer. This means that, for example, malloc (sizeof (place_t)) just allocates space for the pointer, and not for the whole place_t. Do not hide structure names as pointers with typedef, but if necessary, change the malloc operator to

  place=(place_t)malloc(sizeof *place)*num); 

Another problem is that you free () the structure and then try to access the member inside it, which is not valid.

 free(place); free(place->name); 

You have to do it in that order

 free(place->name); free(place); 

You also skip memory, for example. Here: for (i = 0; iname = (char *) malloc (sizeof (char) * MAX);

What will happen in this time loop 2.? You assign place->name , losing the pointer to the memory that you allocated in the first iteration of the loop. Perhaps you wanted to use the place [num] inside the loop, since you are trying to allocate a lot of place_t in place = (place_t) malloc (sizeof (place_t) * num); statement.

Answer 2

The problem is not so much with the malloc statements, but in the order in which you free the memory when you are done with it.

eg.

 free(place); free(place->name); 

it should be

 free(place->name); free(place); 

otherwise, the memory allocated for name will not be freed.

Similarly, calling free(place); not free memory allocated in place->name=(char *)malloc(sizeof(char)*MAX);

You must manually free the memory allocated for each Malloc statement.

It is always better to check if pointers to memory cells are null before reusing them with malloc .

If they are not null , then free them, then set them to null . This will help ensure no memory leaks.