For some reason I end up with memory leak...

This is a discussion on For some reason I end up with memory leak... within the C Programming forums, part of the General Programming Boards category; I'm not sure how I'm freeing the memory incorrectly. I freed all the memory in each string until null, but ...

  1. #1
    Registered User
    Join Date
    Nov 2008
    Posts
    21

    For some reason I end up with memory leak...

    I'm not sure how I'm freeing the memory incorrectly. I freed all the memory in each string until null, but it won't let me free the next level of pointers.....



    Code:
    /*****************************************************************
    ** CIS 15 BG
    ** Fall, 2008
    ***************
    **
    ** Lab 7            Structures           20 Points
    **
    **************************************************
    
       This program uses an array of country structures 
       to answer inquiries.
    
    ****************************************
    **
    **  Written By:  Ryu Komiyama
    **
    **  Date: November 23, 2008
    **
    ******************************************************************/
    #include <stdio.h>
    #include <crtdbg.h>
    #include <stdlib.h>
    #include <string.h>
    #include <ctype.h>
    #define FLUSH while (getchar( ) != '\n')
    #define FILE_IN "countries.txt"
    
    // Type Definitions
    typedef struct {
    	char *id;
    	char *name;
    	char *capital;
    	char *population;
    	} COUNTRY;
    
    // Prototype Declarations
    char getOption ( void );
    void inputFile ( COUNTRY *list, int *numCountries );
    char **createDynAry ( int numCountries );
    void processOption ( char option, COUNTRY *list, int numCountries );
    void insertCommas ( COUNTRY *pWalker, COUNTRY *pLast );
    void printList ( COUNTRY *list, int numCountries );
    void searchID ( COUNTRY *list, int numCountries );
    void searchCapital ( COUNTRY *list );
    void printPop ( COUNTRY *list, int numCountries );
    void releaseMemory ( COUNTRY *list );
    
    int main ( void )
    {
    //  Local Definitions
    	char option = ' ';
    	COUNTRY countryList[30];
    	int numCountries = 0;
    
    //  Statements
        printf("\t\t LAB 7 - STRUCTURES and SEARCHING\n\n");
    
        inputFile( countryList, &numCountries );
    	while ( option != 'Q' )
    	{
    		option = getOption ( );
    		if ( option == 'Q' )
    			break;
    		processOption ( option, countryList, numCountries );
    	}
    	// free memory here!
    	releaseMemory ( countryList );
    	
    
    	printf("\n\t\tEnd of LAB 7"
    		   "\n\t\tHave a great day!\n");
    
        printf( _CrtDumpMemoryLeaks() ? "Memory Leak\n" : "No Leak\n");
    
        return 0;
    }// main
    
    char getOption ( void )
    {
    // Local Declarations
    	char option = '\0';
    
    // Statements
    	printf("\n\t\tCountry List Menu\n");
    	printf("L - list the countries in ID sequence\n");
    	printf("P - list the countries by population in descending order\n");
    	printf("S - Search by ID\n");
    	printf("C - Search by capital\n");
    	printf("Q - Quit the program\n\n");
    	printf("Please enter an option and then hit enter: ");
    	while ( !option )
    	{
    		scanf("&#37;c", &option);
    		FLUSH;
    		if ( toupper(option) == 'L' || toupper(option) == 'P' ||
    			 toupper(option) == 'S' || toupper(option) == 'C' ||
    			 toupper(option) == 'Q')
    			printf("You have selected option \"%c\"\n", toupper(option));
    		else
    		{
    			option = '\0';
    			printf("That option does not exist. Please try again.\n");
    		}
    	}
       return toupper(option);
    }
    
    void inputFile ( COUNTRY *list, int *numCountries )
    {
    // Local Declarations
    	FILE *spIn;
    	COUNTRY *pWalker;
    	COUNTRY *pLast;
    	char **tempInt, **tempName, **tempCap, **tempPop;
    	int i = 0;
    // Statements
    	if ( ! ( spIn = fopen ( FILE_IN, "r" ))) {
    		printf("Unable to access input file.\n");
    		exit(100);
    	}
    	fscanf( spIn, "%d", numCountries );
    	pLast = list + *numCountries - 1;
    	tempInt = createDynAry ( *numCountries );
    	tempName = createDynAry ( *numCountries );
    	tempCap = createDynAry ( *numCountries );
    	tempPop = createDynAry ( *numCountries );
    	for ( pWalker = list; pWalker <= pLast; pWalker++ ) {
    		if ( ! ( tempInt[i] = ( char* ) calloc ( 3, sizeof ( char )))) {
    			printf("Memory unavailable.\n");
    			exit(200);
    		}
    		if ( ! ( tempName[i] = ( char* ) calloc ( 21, sizeof ( char )))) {
    			printf("Memory unavailable.\n");
    			exit(200);
    		}
    		if ( ! ( tempCap[i] = ( char* ) calloc ( 16, sizeof ( char )))) {
    			printf("Memory unavailable.\n");
    			exit(200);
    		}
    		if ( ! ( tempPop[i] = ( char* ) calloc ( 16, sizeof ( char )))) {
    			printf("Memory unavailable.\n");
    			exit(200);
    		}
    		fscanf( spIn, "%2s %20[^;] %*c %15[^;] %*c %s",
    				tempInt[i], tempName[i], tempCap[i], tempPop[i]);
    		pWalker->id = tempInt[i];
    		pWalker->name = tempName[i];
    		pWalker->capital = tempCap[i];
    		pWalker->population = tempPop[i];
    		i++;
    		insertCommas ( pWalker, pLast );
    	}
    	pWalker->id = '\0';
    	pWalker->name = '\0';
    	pWalker->capital = '\0';
    	pWalker->population = '\0';
       return;
    }
    
    char **createDynAry ( int numCountries )
    {
    // Local Declarations
    	char **ary;
    // Statements
    	if ( ! ( ary = ( char** ) calloc ( numCountries, sizeof ( char* )))) {
    		printf("Memory unavailable.\n");
    		exit(200);
    	}
    	ary[numCountries] = '\0';
    	return ary;
    }
    
    void processOption ( char option, COUNTRY *list, int numCountries )
    {
    // Statements
    	switch ( option )
    	{
    	case 'L':	printList( list, numCountries );
    				break;
    	case 'P':	printPop ( list, numCountries );
    				break;
    	case 'S':	searchID( list, numCountries );
    				break;
    	case 'C':	searchCapital( list );
    				break;
    	default:	printf("Fatal error. Variable 'option' is corrupt.\n");
    				exit(300);
    	}
       return;
    }
    
    void insertCommas ( COUNTRY *pWalker, COUNTRY *pLast )
    {
    // Local Declarations
    	char printString[15];
    	char tempString[15];
    	char insertComma[] = ",";
    	int strLength;
    	int checkRmdr;
    	int i;
    // Statements
    	*printString = '\0';
    	strcpy(tempString, pWalker->population);
    	strLength = strlen ( tempString );
    	checkRmdr = strLength % 3;
    	if ( checkRmdr > 0 )
    	{
    		strncat ( printString, tempString, checkRmdr );
    		strcat ( printString, insertComma );
    	}
    	for ( i = checkRmdr; i < (int) strlen ( tempString );  )
    	{
    		strncat ( printString, tempString + i, 3 );
    		i += 3;
    		strcat ( printString, insertComma );
    	}
    	printString[strlen(printString) - 1] = '\0';
    	strcpy(pWalker->population, printString);
    	return;
    }
    
    void printList ( COUNTRY *list, int numCountries )
    {
    // Local Declarations
    	COUNTRY *pWalker;
    	COUNTRY *pLast;
    // Statements
    	pWalker = list;
    	pLast = list + numCountries;
    	printf(	"== ==================== ================ ================\n"
    			"ID NAME                 Capital                Population\n"
    			"== ==================== ================ ================\n");
    	for ( pWalker = list; pWalker < pLast ; pWalker++ )
    	{
    		printf("%s %-20s %-15s %17s\n",
    			   pWalker->id, pWalker->name, 
    			   pWalker->capital, pWalker->population);
    	}
    	printf( "======================= ================ ================\n" );
       return;
    }
    
    void searchID ( COUNTRY *list, int numCountries )
    {
    // Local Declarations
    	COUNTRY *pFirst;
    	COUNTRY *pLast;
    	COUNTRY *pMid;
    	char id[3];
    	int found = 0;
    	
    // Statements
    	pFirst = list;
    	pLast = list + numCountries - 1;
    	printf("Enter the ID: ");
    	gets(id);
    	while ( strcmp(id, pFirst->id ) >= 0 && strcmp(id, pLast->id ) <= 0 ) {
    		pMid = pFirst + (pLast - pFirst) / 2;
    		
    		if ( strcmp ( pMid->id, id ) < 0 )
    			pFirst = ++pMid;
    		if ( strcmp ( pMid->id, id ) > 0 )
    			pLast = --pMid;
    		if ( strcmp ( pMid->id, id ) == 0 )
    		{
    			found = 1;
    			break;
    		}
    	}
    	if ( found ) {
    		printf(	"\n        ID: %s\n"
    				"      Name: %s\n"
    			    "   Capital: %s\n"
    				"Population: %s\n", pMid->id, pMid->name,
    									pMid->capital, pMid->population);
    	}
    	else
    		printf("No matching country. Please try again.\n");
       return;
    }
    
    void searchCapital ( COUNTRY *list )
    {
    // Local Declarations
    	char capital[16];
    	COUNTRY *pWalker;
    	int found = 0;
    	
    // Statements
    	printf("Enter the capital: ");
    	gets( capital );
    	pWalker = list;
    	while ( pWalker->capital )
    	{
    		if ( strcmp ( capital, pWalker->capital ) == 0 )
    		{
    			found = 1;
    			break;
    		}
    		pWalker++;
    	}
    	if ( found ) {
    		printf(	"\n        ID: %s\n"
    				"      Name: %s\n"
    			    "   Capital: %s\n"
    				"Population: %s\n", pWalker->id, pWalker->name,
    									pWalker->capital, pWalker->population);
    	}
    	else
    		printf("Capital is not in the database. Please try again.\n");
    	return;
    }
    
    void printPop ( COUNTRY *list, int numCountries )
    {
    // Local Declarations
    	int sorted;
    	int walker = 0;
    	int last = numCountries - 1;
    	int lastValue;
    	int nextValue;
    	COUNTRY temp;
    	int i;
    // Statements
    	do
    	{
    		sorted = 1;
    		last = numCountries - 1;
    		while ( walker < last )
    		{
    			lastValue = 0;
    			nextValue = 0;
    			i = 0;
    			while ( list[last].population[i] != '\0' )
    			{
    				lastValue += list[last].population[i];
    				i++;
    			}
    			i = 0;
    			while ( list[last-1].population[i] != '\0' )
    			{
    				nextValue += list[last-1].population[i];
    				i++;
    			}
    
    			if ( lastValue > nextValue )
    			{
    				temp = list[last];
    				list[last] = list[last-1];
    				list[last-1] = temp;
    				sorted = 0;
    			}
    			last--;
    		}
    		walker++;
    	} while ( !sorted );
    	printList(list, numCountries);
       return;
    }
    
    void releaseMemory ( COUNTRY *list )
    {
    // Local Declarations
    	int i;
    
    // Statements
    	i = 0;
    	while ( list[i].id )
    		free ( list[i++].id );
    	i = 0;
    	while ( list[i].name )
    		free ( list[i++].name );
    	i = 0;
    	while ( list[i].capital )
    		free ( list[i++].capital );
    	i = 0;
    	while ( list[i].population )
    		free ( list[i++].population );
    
    
       return;
    }
    Last edited by RaDeuX; 11-25-2008 at 10:43 PM.

  2. #2
    Jack of many languages Dino's Avatar
    Join Date
    Nov 2007
    Location
    Katy, Texas
    Posts
    2,309
    Please
    show
    error
    messages.
    Mac and Windows cross platform programmer. Ruby lover.

    Quote of the Day
    12/20: Mario F.:I never was, am not, and never will be, one to shut up in the face of something I think is fundamentally wrong.

    Amen brother!

  3. #3
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Code:
              Country List Menu
    L - list the countries in ID sequence
    P - list the countries by population in descending order
    S - Search by ID
    C - Search by capital
    Q - Quit the program
    
    Please enter an option and then hit enter: q
    You have selected option "Q"
    
                    End of LAB 7
                    Have a great day!
    Memory Leak
    Press any key to continue . . .

  4. #4
    Woof, woof! zacs7's Avatar
    Join Date
    Mar 2007
    Location
    Australia
    Posts
    3,459
    Code:
    void releaseMemory ( COUNTRY *list )
    {
    // Local Declarations
    	int i;
    
    // Statements
    	for ( i = 0; i <= 21; i++ )
    	{
    		free ( list[i].id );
    		free ( list[i].name );
    		free ( list[i].capital );
    		free ( list[i].population );
    	}
    
       return;
    }
    Where does this magic 21 come from? free()'ing non-NULL pointers that you didn't allocate is very bad.

    Also don't use gets() -- see the FAQ.
    Last edited by zacs7; 11-25-2008 at 09:09 PM.

  5. #5
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Quote Originally Posted by zacs7 View Post
    Code:
    void releaseMemory ( COUNTRY *list )
    {
    // Local Declarations
    	int i;
    
    // Statements
    	for ( i = 0; i <= 21; i++ )
    	{
    		free ( list[i].id );
    		free ( list[i].name );
    		free ( list[i].capital );
    		free ( list[i].population );
    	}
    
       return;
    }
    Where does this magic 21 come from? free()'ing non-NULL pointers that you didn't allocate is very bad.

    Also don't use gets() -- see the FAQ.
    I just put in 21 to see whether or not I freed the memory correctly. 21 is numCountries -1. The list's variables carry pointers to tempInt, tempName, tempCap, and tempPop. I also don't like using fgets in this situation because it carries over the '\n' as well.
    Last edited by RaDeuX; 11-25-2008 at 10:34 PM.

  6. #6
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Okay, I fixed releaseMemory to be more NULL friendly.

    Code:
    void releaseMemory ( COUNTRY *list )
    {
    // Local Declarations
    	int i;
    
    // Statements
    	i = 0;
    	while ( list[i].id )
    		free ( list[i++].id );
    	free ( list[i].id );
    	i = 0;
    	while ( list[i].name )
    		free ( list[i++].name );
    	free ( list[i].name );
    	i = 0;
    	while ( list[i].capital )
    		free ( list[i++].capital );
    	free ( list[i].capital );
    	i = 0;
    	while ( list[i].population )
    		free ( list[i++].population );
    	free ( list[i].population );
    
       return;
    }
    Last edited by RaDeuX; 11-25-2008 at 10:47 PM.

  7. #7
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,484
    1. Pass numCountries to your release function.
    2. Call inputFile( countryList, &numCountries ); only ONCE.

    You're allocating space for the whole file every time around the loop, so unless the first thing you do is quit, it's leaking.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  8. #8
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Quote Originally Posted by Salem View Post
    1. Pass numCountries to your release function.
    2. Call inputFile( countryList, &numCountries ); only ONCE.

    You're allocating space for the whole file every time around the loop, so unless the first thing you do is quit, it's leaking.
    As long as I free the memory in a while loop 'til it hits NULL then I wouldn't need numCountries right? I'm not sure if releaseMemory is freeing the first level dynamically allocated pointers or the second level pointers...
    Last edited by RaDeuX; 11-25-2008 at 10:56 PM.

  9. #9
    Woof, woof! zacs7's Avatar
    Join Date
    Mar 2007
    Location
    Australia
    Posts
    3,459
    That's fine if you don't want to use fgets() -- the FAQ explains how to remove the \n. Just DON'T use gets(). You're post is about a memory leak, yet you have a potential buffer overrun (which could also cause a memory leak).

    > I'm not sure if releaseMemory is freeing the first level dynamically allocated pointers or the second level pointers...
    Perhaps you need to brush up on your pointers then...

  10. #10
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Well it's under the assumption that the user correctly defines what they want. But anyway...

    It's just that structures makes it a little different. I'm okay with pointers but I'm not godlike at it.

  11. #11
    Registered User
    Join Date
    Nov 2008
    Posts
    21
    Found it. I completely forgot that stacks and heap are different. I removed the following code and made the appropriate changes.

    Code:
    tempInt = createDynAry ( *numCountries );
    tempName = createDynAry ( *numCountries );
    tempCap = createDynAry ( *numCountries );
    tempPop = createDynAry ( *numCountries );

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Problem building Quake source
    By Silvercord in forum Game Programming
    Replies: 16
    Last Post: 07-11-2010, 09:13 AM
  2. Mutex and Shared Memory Segment Questions.
    By MadDog in forum Linux Programming
    Replies: 14
    Last Post: 06-20-2010, 04:04 AM
  3. Memory leak prevention methodogies
    By c___newbie in forum A Brief History of Cprogramming.com
    Replies: 17
    Last Post: 11-17-2007, 02:45 AM
  4. memory leak
    By rahulsk1947 in forum C Programming
    Replies: 2
    Last Post: 11-11-2007, 12:27 PM
  5. Is this a memory leak?
    By cboard_member in forum C++ Programming
    Replies: 9
    Last Post: 07-20-2005, 01:15 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21