Problem with memory free in C

[GDICommander]

Hello, everyone!

I have a problem with memory free. Here is the necessary code to understand the problem:

void fillArray(char** referenceArray, FILE* grid)
{
	//Chaîne qui va contenir une ligne du fichier.
	char* lineContent = malloc((width + 1) * sizeof(char));		//Pour contenir le "\0"
	
	int i, j;

	for (i = 0; i < height; ++i)
	{
		printf("Iteration nb %d\n", i);
		referenceArray[i] = malloc(width * sizeof(char));	//Allocation de la mémoire pour une ligne du tableau.
		fgets(lineContent, width + 1, grid);	//width + 1 pour aller chercher "width" caractères.
		printf("String: %s\n", lineContent);

		for (j = 0; j < width; ++j)
		{
			referenceArray[i][j] = lineContent[j];
		}

		fgets(lineContent, 2, grid);		//On se débarasse du "\n" au bout.
	}

	free(lineContent);
}


char** referenceArray;
char** newReferenceArray;

referenceArray = malloc(height * sizeof(char*));
fillArray(referenceArray, grid);

do
	{
		survivorCount = 0;

		newReferenceArray = computeNextGen(&survivorCount);

		printf("Survivor count: %d\n", survivorCount);

		//TODO: Memory free problems!
		for (i = 0; i < height; ++i)
		{
			free(referenceArray[i]);
		}
		free(referenceArray);

		referenceArray = newReferenceArray;

		++generationCount;
	}
	while (survivorCount != 0 && generationCount < generationLimit);

And here is the computeNextGen function.

char** computeNextGen(int* survivorCount)
{
	int i = 0;
	int j = 0;

	char cellStatus;

	char** arrayToReturn = malloc(height * sizeof(char));
	for (i = 0; i < height; ++i)
	{
		arrayToReturn[i] = malloc(width * sizeof(char));
	}

        return arrayToReturn;
}

The problem happens at the iterations of the Life Game that follows the first one. Microsoft Visual Studio complains on runtime at the free(referenceArray) in the do-while loop. Looking at the code makes me blind for finding the mistake.

I hope that I have put enough code in my project to help you understand my problem. Putting a huge load of code is not very convenient for reading, I know...

Thank you for your help, if you know the solution.

[Dave Sinkula]

Microsoft Visual Studio complains on runtime at the free(referenceArray) in the do-while loop.

Are you trying to free it more than once?

[GDICommander]

I don't think that I'm freeing the 2D array twice, because it is filled first in the fillArray() function. The referenceArray pointer is cleaned first and points to the newReferenceArray pointer. Then newReferenceArray becomes a new reference array again. The "referenceArray" pointer is supposed to delete the memory of the old new referenceArray and to point on the new referenced array.

Note: I have updated the code with the fillArray() function.

[Dave Sinkula]

I prefer a standalone compilable snippet, when possible. Otherwise I just browse bits and pieces and don't really see your issue.

char** arrayToReturn = malloc(height * sizeof(char));

Are you sure the multiplication is by the correct size?

[edit]I prefer this idiom to avoid potential issues:

p = malloc ( n * sizeof *p );

[GDICommander]

Ok, here is the complete code.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

//Variables globales.
int width;
int height;

const char ALIVE = '1';
const char DEAD = '0';

char** referenceArray;

void fillArray(char** referenceArray, FILE* grid)
{
	//Chaîne qui va contenir une ligne du fichier.
	char* lineContent = malloc((width + 1) * sizeof(char));		//Pour contenir le "\0"
	
	int i, j;

	for (i = 0; i < height; ++i)
	{
		printf("Iteration nb %d\n", i);
		referenceArray[i] = malloc(width * sizeof(char));	//Allocation de la mémoire pour une ligne du tableau.
		fgets(lineContent, width + 1, grid);	//width + 1 pour aller chercher "width" caractères.
		printf("String: %s\n", lineContent);

		for (j = 0; j < width; ++j)
		{
			referenceArray[i][j] = lineContent[j];
		}

		fgets(lineContent, 2, grid);		//On se débarasse du "\n" au bout.
	}

	free(lineContent);
}

/**
	\brief Détermine si une cellule est vivante.
*/
int isAlive(char* cell)
{
	return (*cell == ALIVE) ? 1 : 0;
}

/**
	\brief Détermine si une cellule est morte.
*/
int isDead(char* cell)
{
	return (*cell == DEAD) ? 1 : 0;
}

/**
	\brief Compte le nombre de voisins d'une cellule.
	\param i	Rangée i de la cellule.
	\param j	Colonne j de la cellule.
	\return	Le nombre de voisins.
*/
int countNeighbors(int i, int j)
{
	int up = i-1;
	int bottom = i+1;
	int left = j-1;
	int right = j+1;

	int neighborCount = 0;

	int currentI, currentJ;

	//Si on est à gauche du tableau, on n'ira pas vérifier à gauche de la cellule.
	if (left < 0)
		left = 0;
	
	//Si on est en haut du tableau, on n'ira pas vérifier en haut de la cellule.
	if (up < 0)
		up = 0;

	//Si on est à droite du tableau, on n'ira pas vérifier à droite de la cellule.
	if (right > width - 1)
		right = width - 1;

	//Si on est en bas du tableau, on n'ira pas vérifier à droite de la cellule.
	if (bottom > height - 1)
		bottom = height - 1;

	//On compte le nombre de voisins.
	for (currentI = up; currentI <= bottom; ++currentI)
	{
		for (currentJ = left; currentJ <= right; ++currentJ)
		{
			//On ne veut pas compter la cellule au milieu!
			if (!(currentI == i && currentJ == j))
			{
				if (isAlive(&referenceArray[currentI][currentJ]))
				{
					++neighborCount;
				}
			}
		}
	}

	return neighborCount;
}

/**
	Détermine si une cellule à une position précise devient vivante ou morte
	à la prochaine génération.
*/
char determineCellFuture(char* cell, int i, int j)
{
	//On compte le nombre de voisins.
	char retval = *cell;

	int nbNeighbors = countNeighbors(i, j);

	if (isAlive(cell))
	{
		if (nbNeighbors < 2)
		{
			retval = DEAD;
		}
		else if (nbNeighbors > 3)
		{
			retval = DEAD;
		}
	}
	else if (isDead(cell))
	{
		if (nbNeighbors == 3)
		{
			retval = ALIVE;
		}
	}

	return retval;
}

/**
	\brief	Fournit un tableau qui indique la prochaine génération du tableau.
*/
char** computeNextGen(int* survivorCount)
{
	int i = 0;
	int j = 0;

	char cellStatus;

	//Allocation du tableau qui sera retourné.
	char** arrayToReturn = malloc(height * sizeof(char));
	for (i = 0; i < height; ++i)
	{
		arrayToReturn[i] = malloc(width * sizeof(char));
	}

	//On commence à passer sur chacune des cellules pour trouver son statut
	//à la prochaine itération.
	for (i = 0; i < height; ++i)
	{
		for (j = 0; j < width; ++j)
		{
			cellStatus = determineCellFuture(&referenceArray[i][j], i, j);

			if (cellStatus == ALIVE)
				++(*survivorCount);

			arrayToReturn[i][j] = cellStatus;
		}
	}

	printf("About to return...\n");
	//Affichage des valeurs du tableau de référence.
	for (i = 0; i < height; ++i)
	{
		for (j = 0; j < width; ++j)
		{
			printf("%c ", arrayToReturn[i][j]);
		}
		printf("\n");
	}

	return arrayToReturn;
}

int main(int argc, char* argv[])
{
	//TODO: Faire la gestion des arguments d'entrée du programme de façon plus robuste.
	
	FILE* grid;

	//La taille des 2 chaînes ne devrait pas dépasser 10 bytes.
	char* widthStr;
	char* heightStr;

	char** newReferenceArray;

	int i;

	int survivorCount = 0;
	int generationCount = 0;

	int generationLimit = atoi(argv[1]);
	grid = fopen(argv[2], "r");

	widthStr = (char*) malloc(5 * sizeof(char));
	heightStr = (char*) malloc(5 * sizeof(char));

	//Lecture des dimensions de la grille initiale.
	fgets(heightStr, 5, grid);
	fgets(widthStr, 5, grid);

	//On enlève le "\n" au bout des dimensions.
	heightStr[strlen(heightStr) - 1] = '\0';
	heightStr[strlen(widthStr) - 1] = '\0';
	
	//On obtient les dimensions dans des int.
	width = atoi(widthStr);
	height = atoi(heightStr);

	//On n'a plus besoin des tableaux qui contiennent les dimensions...
	free(widthStr);
	free(heightStr);

	//Remplissage du tableau de référence.

	//Allocation de la mémoire pour ce tableau.
	referenceArray = malloc(height * sizeof(char*));

	fillArray(referenceArray, grid);

	//Commencer l'algorithme...
	//Parcours de chaque cellule du tableau.

	do
	{
		survivorCount = 0;

		newReferenceArray = computeNextGen(&survivorCount);

		printf("Survivor count: %d\n", survivorCount);

		//Libération de la mémoire du tableau de référence.
		//TODO: Problèmes de libération de mémoire!
		for (i = 0; i < height; ++i)
		{
			free(referenceArray[i]);
		}
		free(referenceArray);		//TODO: Est-ce un "leak" de mémoire?

		//La nouvelle référence devient le nouveau tableau de référence.
		referenceArray = newReferenceArray;

		++generationCount;
	}
	while (survivorCount != 0 && generationCount < generationLimit);

	return 0;
}

This program needs arguments for running and an input file.

For the first argument, put 20. It should be ok.

For the file, copy/paste this and specify the name as your second argument:

10
10
0000000000
0000000000
0000000000
0000100000
0000010000
0001110000
0000000000
0000000000
0000000000
0000000000

I have noticed that I malloc'd the referenceArray of the main() with a sizeof(char*), that is not correct. What I really want is sizeof(char), but it didn't correct the problem.

[Dave Sinkula]

Thanks for posting the whole source. Did the change I mentioned in #4 fix your issue?

[GDICommander]

Unfortunately, no. Now I have a runtime error on the free() call at the last line of fillArray(). The change of char* to char for referenceArray has introducted a new problem in the fillArray() function. I should look at it before debugging other sections of code.

[Dave Sinkula]

The change of char* to char for referenceArray has introducted a new problem in the fillArray() function.

I don't know if I read you correctly. My hint towards a suggested replacement would be:

char **arrayToReturn = malloc(height * sizeof *arrayToReturn);

[edit]This being your original:

char** arrayToReturn = malloc(height * sizeof(char));

[wildgoose]

Just briefly unless your assignment is to use a malloc() you don't need it here...

widthStr = (char*) malloc(5 * sizeof(char));
	heightStr = (char*) malloc(5 * sizeof(char));

	//Lecture des dimensions de la grille initiale.
	fgets(heightStr, 5, grid);
	fgets(widthStr, 5, grid);

	//On enlève le "\n" au bout des dimensions.
	heightStr[strlen(heightStr) - 1] = '\0';
	heightStr[strlen(widthStr) - 1] = '\0';

Instead do...

char widthStr[ 5 ], heightStr[ 5 ];

fgets(heightStr, 5, grid);
fgets(widthStr, 5, grid);

//	heightStr[strlen(heightStr) - 1] = '\0';
//	heightStr[strlen(widthStr) - 1] = '\0';

Also note you don't need that set terminator as fgets() actually reads length -1 then sets the terminator itself. So since you specified (5), four bytes are read and the terminator is set.

Are you aware there's a calloc() memory allocation call? Designed for two dimensional array allocation?

char** arrayToReturn = malloc(height * sizeof(char));
should be
char** arrayToReturn = malloc(height * sizeof(char *));

You're allocating the backbone that you're attaching (height) allocated memory blocks to! So char *, not char.

Well enough for tonight!

[GDICommander]

Hello everyone! It's me again.

I replaced malloc calls with calloc and I put sizeof(char*) for the memory allocation call for the rows of the 2D array. There are no runtime errors.

Thank you for introducing me to calloc and for helping me with memory allocation in C!

[Dave Sinkula]

http://c-faq.com/malloc/calloc.html

[jephthah]

yeah, i was going to say about the same thing....

because obviously there must have been more of a change than merely replacing malloc() with calloc(), since the two are functionally identical aside from initializing memory with zeroes.

.