Thread: Memory leak issues! Please help! >_<

For some reason I'm getting a memory leak error. It's probably from the part where I start to copy the strings and allocate an array of characters, but I have no idea why. I'm using a text file for input data, which is found here: http://puma.deanza.edu/distribute/De.../countries.txt

I appreciate anyone who helps me solve this issue :]

Code:

#include <stdio.h>
#include <stdlib.h>
#include <crtdbg.h>
#define FILE_IN "countries.txt" // user is free to change filename

// Prototype Declarations
char **getNames ( void );

int main (void)
{
//  Local Definitions 
	char **countryList;
	int i;

//  Statements 
	printf("\t\t LAB 6 - Strings\n\n" );
	countryList = getNames();
	//for (i=0;i<=13;i++)
		//free(countryList[i]);
	free(countryList);

    printf( _CrtDumpMemoryLeaks() ? "Memory Leak\n" : "No Leak\n");
	printf("\n\t\tEnd of Lab 6\n"
		   "\n\t\tHave a great day!\n");
   return 0;

} // main

char **getNames ( void )
{
// Local Declarations
	FILE *spIn;
	int numCountries = 0;
	char tempAry[50];
	char **countryList;
	int i;
	int closeResult;

// Statements
	if ( ! ( spIn = fopen ( FILE_IN , "r" ) ) )
	{
		printf("Error loading input file.\n");
		exit(100);
	}
	while(fgets(tempAry, sizeof(tempAry), spIn))
		numCountries++;
	printf("Number of countries: %d\n", numCountries);
	closeResult = fclose ( spIn );
	if (closeResult==EOF)
	{
		printf("File did not close properly.\n");
		exit(101);
	}
	if ( ! ( spIn = fopen ( FILE_IN , "r" ) ) )
	{
		printf("Error loading input file.\n");
		exit(102);
	}
	if ( ! ( countryList = (char**) calloc (numCountries + 1, sizeof(char*))))
	{
		printf("Memory is unavailable.\n");
		exit(103);
	}
	for ( i = 0; i < numCountries; i++ )
	{
		fgets(tempAry, sizeof(tempAry), spIn);
		countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));
		strcpy(countryList[i], tempAry);
		printf("%s", countryList[i]);
	}
	countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));
	countryList[numCountries] = '\0';

	closeResult = fclose ( spIn );
	if (closeResult==EOF)
	{
		printf("File did not close properly.\n");
		exit(104);
	}

   return countryList;
} // getNames

Because you commented out the part where you returned all the memory?

Originally Posted by tabstop

Because you commented out the part where you returned all the memory?

My compiler gives me a heap stack error when I run that.

Do you have 14 countries? In fact, when you get back to main, how do you know how many countries you have? (Answer: You have an empty country, so loop until you get to that blank country, not until you have 14 countries.)

The last country is a null pointer. Our teacher wants us to put a null pointer at the end of any array of pointers. I just put the numbers there assuming that the parameters would pass the value of the number of countries just to speed things up.

Originally Posted by RaDeuX

I just put the numbers there assuming that the parameters would pass the value of the number of countries just to speed things up.

That may be the silliest thing I've ever seen. You expected the compiler to replace the literal value 13 with the value of i from another function?

Originally Posted by tabstop

That may be the silliest thing I've ever seen. You expected the compiler to replace the literal value 13 with the value of i from another function?

That's not the point. The 13 is temporary. What I'm trying to fix is the memory leak, not the functionality of the program.

Originally Posted by iMalc

Code:

		countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));

You need +1 for the NULL there.

No I don't. i is 13 after the for loop ends.

Originally Posted by RaDeuX

That's not the point. The 13 is temporary. What I'm trying to fix is the memory leak, not the functionality of the program.

Then free all your memory.

Code:

i = 0;
while (countryList[i][0] != '\0') {
    free(countryList[i]);
    i++;
}
free(countryList[i]); /* the one at the end that is the \0 character */
free(countryList); /* now you can do such a thing */

Originally Posted by tabstop

Then free all your memory.

Code:

i = 0;
while (countryList[i][0] != '\0') {
    free(countryList[i]);
    i++;
}
free(countryList[i]); /* the one at the end that is the \0 character */
free(countryList); /* now you can do such a thing */

That causes the same debug error. It tells me that its detected a heap corruption.

Here's the updated version:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <crtdbg.h>
#define FILE_IN "countries.txt" // user is free to change filename

// Prototype Declarations
char **getNames ( int *numCountries );

int main (void)
{
//  Local Definitions
	char **countryList;
	int numCountries = 0;
	int i;

//  Statements 
	printf("\t\t LAB 6 - Strings\n\n" );
	countryList = getNames(&numCountries);
	//for (i=0;i<=numCountries;i++)
		//free(countryList[i]);
	
	free(countryList);
	
    printf( _CrtDumpMemoryLeaks() ? "Memory Leak\n" : "No Leak\n");
	printf("\n\t\tEnd of Lab 6\n"
		   "\n\t\tHave a great day!\n");
   return 0;

} // main

char **getNames ( int *numCountries )
{
// Local Declarations
	FILE *spIn;
	char tempAry[50];
	char **countryList;
	int i;
	int closeResult;

// Statements
	if ( ! ( spIn = fopen ( FILE_IN , "r" ) ) )
	{
		printf("Error loading input file.\n");
		exit(100);
	}
	while(fgets(tempAry, sizeof(tempAry), spIn))
		(*numCountries)++;
	printf("Number of countries: &#37;d\n", *numCountries);
	closeResult = fclose ( spIn );
	if (closeResult==EOF)
	{
		printf("File did not close properly.\n");
		exit(101);
	}
	if ( ! ( spIn = fopen ( FILE_IN , "r" ) ) )
	{
		printf("Error loading input file.\n");
		exit(102);
	}
	if ( ! ( countryList = (char**) calloc (*numCountries + 1, sizeof(char*))))
	{
		printf("Memory is unavailable.\n");
		exit(103);
	}
	for ( i = 0; i < *numCountries; i++ )
	{
		fgets(tempAry, sizeof(tempAry), spIn);
		countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));
		strcpy(countryList[i], tempAry);
		printf("%s", countryList[i]);
	}

	countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));
	countryList[*numCountries] = '\0';

	closeResult = fclose ( spIn );
	if (closeResult==EOF)
	{
		printf("File did not close properly.\n");
		exit(104);
	}
	
   return countryList;
} // getNames

So writing past the end of the heap is going to be an error in the writing part of the whole deal, and looking there, yes you manage to calloc one less character than you need every single time through. So fix that.

Also, this:

Code:

	countryList[i] = (char*) calloc (strlen(tempAry), sizeof(char));
	countryList[*numCountries] = '\0';

is a guaranteed memory leak, since i and *numCountries will be the same at this point -- you calloc some memory and then set the pointer to NULL. If you want the pointer to be NULL then don't calloc any memory (and then you don't have to free it back in main). If you want the pointer to exist, but point to an empty string, then you need to set countryList[i][0] to '\0' instead.

I didn't know '/n' counted as a character as well, which was part of the reason why I didn't have that extra character space. Thanks for your input, there are no more memory leaks now.