Thread: Heap Corruption with calloc'ed 2D arrays

I keep getting a heap corruption with my code below. I have attached my full code if anyone whats to run it. Essentially, I calloc a 2D array in 'main', pass it to 'inputCmd', which calloc's out memory for some strings, fills the memory up, and then realloc's the space to shrink it to the desired size. I have it passing-by-reference successfully, but I noticed when 'inputCmd' calloc's memory around the 2nd or 3rd iteration in the loop, it creates an extra pointer that points to 0x21 (which, of course, is unusable since that memory is reserved for the OS). This can later get reassigned by the next calloc in the loop, but by the end there is always one or two extra pointers that are pointing to memory that cannot be read. When I run 'printData' and 'terminate' (other functions in my full source code), my program crashes and says there is a memory error. I don't understand what is wrong.

Code:

void inputCmd(char **data, returnValue *condition)
{
	int i = 0;
	int arrayLength = 0;

	printf("How Many Elements Are You Entering? ");
	fscanf(stdin, "%d", &arrayLength);

	/*Read in last '\n' from stdin to empty out the stream*/
	getc(stdin);

	if(arrayLength <= 0)
	{
		printf("Could Not Read Data.\n");
		condition->status = NOREAD;
	}
	else
	{
		for(i = 0; i < arrayLength; i++)
		{
			data[i] = (char *)calloc(wordSize, sizeof(char));

			printf("Enter Data (One At A Time): ");
			fscanf(stdin, "%s", data[i]);

			data[i] = (char *)realloc(data[i], sizeof(char) * (strlen(data[i]) + 1));
		}

		strcpy(condition->returnValue, (char *)&arrayLength);
	}
}

int main(int argc, char* argv[])
{
	FILE *dataFile = NULL;
	char **data = (char **)calloc(1, sizeof(char *));

	char inputLocation[wordSize], fileLocation[wordSize];
	int arrayLength = 0;

	returnValue *condition = (returnValue *)calloc(1, sizeof(returnValue));
	condition->returnValue = (char *)calloc(15, sizeof(char));

	memset(inputLocation, '\0', wordSize);
	memset(fileLocation, '\0', wordSize);

	do
	{
		if(condition->status == OK)
		{
			switch(inputLocation[0])
			{
				case 'c':
				{
					inputCmd(data, condition);
					arrayLength = (int)condition->returnValue[0];

					break;
				}
				default:
					break;
			}
		}
	}
	while(condition->status != OK);

	strcpy(condition->returnValue, (char *)&arrayLength);
	terminate(condition, data);

	/*
	 * This code should never be run, therefore,
	 * 'EXIT_FAILURE' is sent back if it is.
	 */
	return(EXIT_FAILURE);
}

This is probably the culprit:

Code:

char **data = (char **)calloc(1, sizeof(char *));

Here you are allocating enough space for one char*. That means you can access data[0], but not data[1], [2], ... But then in inputCmd(), you essentially do this:

Code:

for(i = 0; i < arrayLength; i++) data[i] = blah;

See what happens if arrayLength is anything greater than 1? You overrun your buffer. You'll need to allocate more space, or if you'd like, realloc() as you keep entering data (to do this you'll need to pass a pointer to your char** so you can reassign it).

I would also recommend removing all casts. Casts should be used sparingly, and almost never to silence the compiler. Most of your casts are harmless (although unnecessary), but look at this:

Code:

strcpy(condition->returnValue, (char *)&arrayLength);

I suspect you added this cast because the compiler was complaining about converting an int* to a char*. It complained with good reason! An int is not a string, so you should not tell the compiler that it is. A cast in C does not do conversions the way you might think. You cannot convert an integer to a string with a cast; what you instead are doing is telling strcpy() that the bytes of your integer represent the bytes of a string. It will dutifully read and copy them until it finds a zero byte. Perhaps there was one in your integer, perhaps not; but either way the copy won't make sense.

If you want to convert an integer to a string, you should use sprintf() (or, if you can, snprintf()). This function is like printf(), but instead of writing to standard output, it writes to a string.

Casting is not inherently wrong, but it is often used where it should not be. Sometimes the cast is simply redundant, but other times it can hide real problems. Your first instinct, when you have a problem, should not be to cast. I wish there were a hard and fast rule for when to cast, but there's not; experience will help teach you when casting is the right thing.

Thanks for your post, I have fixed my casting. But I still have my heap problem. I have also noticed that when 'inputCmd' finishes, the data is not sent back into 'main' though all the new pointers have been allocated (plus the usual extra ones as stated in my original post).

This is my new code. Any ideas?

Code:

void printData(char **data, int arrayLength, returnValue *condition)
{
	int i = 0;

	printf("Sorted Data:\n");

	for(i = 0; i < (arrayLength - 1); i++)
		printf("   %s,\n", data[i]);

	printf("   %s.\n", data[arrayLength - 1]);
}

void terminate(returnValue *condition, char **data)
{
	int i = 0;
	int status = condition->status;

	if(data != NULL)
	{
		for(i = 0; i < condition->returnValue[0]; i++)
		{
			if(data[i] != NULL)
				free(data[i]);
		}

		if(data != NULL)
			free(data);
	}

	if(condition != NULL)
	{
		if(condition->returnValue != NULL)
			free(condition->returnValue);

		free(condition);
	}

	if(status == OK)
		exit(EXIT_SUCCESS);
	else
		exit(EXIT_FAILURE);
}

void inputCmd(char **data, returnValue *condition)
{
	int i = 0;
	int arrayLength = 0;

	printf("How Many Elements Are You Entering? ");
	fscanf(stdin, "%d", &arrayLength);

	/*Read in last '\n' from stdin to empty out the stream*/
	getc(stdin);

	if(arrayLength <= 0)
	{
		printf("Could Not Read Data.\n");
		condition->status = NOREAD;
	}
	else
	{
                data = realloc(data, sizeof(char *) * arrayLength);

		for(i = 0; i < arrayLength; i++)
		{
			data[i] = calloc(wordSize, sizeof(char));

			printf("Enter Data (One At A Time): ");
			fscanf(stdin, "%s", data[i]);

			data[i] = realloc(data[i], sizeof(char) * (strlen(data[i]) + 1));
		}

		sprintf(condition->returnValue, "%d", arrayLength);
	}
}

int main(int argc, char* argv[])
{
	FILE *dataFile = NULL;
	char **data = calloc(1, sizeof(char *));

	char inputLocation[wordSize], fileLocation[wordSize];
	int arrayLength = 0;

	returnValue *condition = calloc(1, sizeof(returnValue));
	condition->returnValue = calloc(15, sizeof(char));

	memset(inputLocation, '\0', wordSize);
	memset(fileLocation, '\0', wordSize);

	do
	{
		condition->status = OK;

		askInputLocation(inputLocation);
		checkInputLocation(inputLocation, condition);

		if(condition->status == OK)
		{
			switch(inputLocation[0])
			{
				case 'c':
				{
					inputCmd(data, condition);
					arrayLength = (int)condition->returnValue[0];

					break;
				}
				default:
					break;
			}
		}

		if(condition->status == OK)
			printData(data, arrayLength, condition);
	}
	while(condition->status != OK);

	sprintf(condition->returnValue, "%d", arrayLength);
	terminate(condition, data);

	/*
	 * This code should never be run, therefore,
	 * 'EXIT_FAILURE' is sent back if it is.
	 */
	return(EXIT_FAILURE);
}

For starters, there's a type and variable name conflict

Code:

returnValue *condition = calloc(1, sizeof(returnValue));   /* returnValue is a type name */
condition->returnValue = calloc(15, sizeof(char));         /* returnValue is a variable name?? */

Thanks, my IDE, Netbeans, was kind enough to catch that problem and differentiate between the variable type and the variable name. I've fixed that. Anything for the memory problems?

The problem is one I mentioned in my first post:

Originally Posted by cas

You'll need to allocate more space, or if you'd like, realloc() as you keep entering data (to do this you'll need to pass a pointer to your char** so you can reassign it).

You're reassigning to “data” in inputCmd(), but that assignment isn't reflected in the caller; it's still pointing to the old value. realloc() might return the same address as it previously used, but there is no requirement to. You have to tell main() that you've changed the value of the pointer.

Once you solve that problem, you're going to have an issue with passing the wrong array size to printData(). You're writing a string to condition->returnValue, which makes sense because that's a char*. Fine. But then you access condition->returnValue[0] which is a single char. Let's say you wrote 10 to the string because you read 10 elements. returnValue[0] will be '1' (the character 1, which has a value of 49 in Latin-1 and similar character sets).

That is, you're writing a string, but then sort of pretending that string is an integer later on. If you absolutely must use a string, then you'll have to convert it back to a number with strtol() or a similar function. Otherwise, you should store an int as an int, not a string. Once again, casting is incorrect here. You're under the assumption (it seems) that casting returnValue[0] to an int will do something useful, but it won't. It's being converted to an int anyway, but not in the manner you would like.

C does not do conversions between strings and integers for you. That's up to you to do with snprintf(), strtol(), and related functions.

And not that it's a bug, per se, but there's no need to test a pointer before passing it to free(); free(NULL) is perfectly legal.

Thank you so much! I have fixed it all, except when I calloc() space for each new strings in my 2d array I get the inevitable extra pointers created pointing to system memory. I have noticed that some of those disappear as I free() the pointers that I have calloc()'ed.

Thank you for your help.