C Programming: freeing array of structs causes heap overflow

[cubanmissile]

Homework Statement

Error:

warning: HEAP[Happy Birthday.exe]: 
warning: Heap block at 006E15A8 modified at 006E1688 past requested size of d8

My Struct:

struct student { // Defines a student.

	char firstName[NAMELENGTH], lastName[NAMELENGTH];
	int day, month, year;

};

My Code (Partial):

void findClosestBirthday(FILE *file) {

	char month[NAMELENGTH], targetStudentFirst[NAMELENGTH], targetStudentLast[NAMELENGTH];
	int classes, students, targets, i, j;
	struct student *class;

	fscanf(file, "%d", &classes);

	for(i = 0; i < classes; i++) { // Loops through all classes.

		fscanf(file, "%d", &students);
		class = (struct student*)malloc(students*sizeof(struct student));

		for(j = 0; j < students; j++) { // Loops through all students.

			fscanf(file, "%s %s %s %d %d", &class[j].firstName[0], &class[j].lastName[0], month, &class[j].day, &class[j].year);
			class[j].month = monthToInt(month);

		}

		sortArray(class, 0, students); // Sorts students.

		fscanf(file, "%d", &targets); // Scans in # of target students.

		printf("Class %d\n", i+1);

		for(j = 0; j < targets; j++) {

			fscanf(file, "%s %s", &targetStudentFirst[0], &targetStudentLast[0]);
			printBirthdays(class, students, targetStudentFirst, targetStudentLast);

		}

		printf("\n");

		free(class); // Frees class.

	}

}

At free(class) the program crashes. I AM modifying the array of structs in this function:

void swap(struct student *array, int i, int j) {

	struct student temp;

	temp = array[i];
	array[i] = array[j];
	array[j] = temp;

}

Homework Equations

None

The Attempt at a Solution

I've tried many.

First, I moved the free(class) to outside the class FOR loop and instead would realloc class based on new scanned in number of students. Would complain about heap overflow.

Secondly, I changed the swap function to this:

void swap(struct student *array, int i, int j) {

	struct student temp;

	// Make temp = array[i].
	strcpy(temp.firstName, array[i].firstName);
	strcpy(temp.lastName, array[i].lastName);
	temp.day = array[i].day;
	temp.month = array[i].month;
	temp.year = array[i].year;

	// Make array[i] = array[j].
	strcpy(array[i].firstName, array[j].firstName);
	strcpy(array[i].lastName, array[j].lastName);
	array[i].day = array[j].day;
	array[i].month = array[j].month;
	array[i].year = array[j].year;

	// Make array[j] = temp
	strcpy(array[j].firstName, temp.firstName);
	strcpy(array[j].lastName, temp.lastName);
	array[j].day = temp.day;
	array[j].month = temp.month;
	array[j].year = temp.year;

}

Still complained about heap overflow. I'm confused as to why? I am sure I am following all practical conventions. I *assume* it has to do with my swap function, but cannot understand how to fix it. :(

If you guys need the entire program let me know, just would rather not post it all due to other students in my class maybe searching for the program so they copy. -_-

Thanks for ANY and ALL help/pointers! Let me know if you need the entire program.

 

[rcgldr]

Although the error was reported when freeing the structs, the actual error seems to be that you wrote data past the end of a structure (excessive index or pointer).

 

[cubanmissile]

Although the error was reported when freeing the structs, the actual error seems to be that you wrote data past the end of a structure (excessive index or pointer).

Wouldn't that call an error when trying to write said index? I'll check that out, you bring up a valid point, thank you! :)

EDIT: You are correct! For some odd reason my sorting function is calling the swap function with an index too big! I didn't think of checking it, that means my sorting function is not good. Thanks a lot for the help. :)

 

[Hurkyl]

char firstName[NAMELENGTH]

As soon as I saw this, my best two guesses for what's wrong with your code are:

• You forgot to account for the trailing null character: such an array can only contain a string of length NAMELENGTH-1.
• You aren't doing adequate bounds checking, so someplace you tried to write a string that was too long

Now, the quoted line above is not wrong, per se: it's a common C paradigm. But unfortunately, there are a good number of bugs and design flaws that are related to it.

fscanf(file, "%s %s %s %d %d", &class[j].firstName[0], &class[j].lastName[0], month, &class[j].day, &class[j].year);

Seeing this line makes me extremely suspicious that point 2 is happening. This code reads in strings from a file without any attempt to verify that the strings are not too long.

Additionally, you don't do any error checking on your file operations, which makes me mildly suspicious that there could have been an error you never noticed, and that's making things behave differently than you expect.

But now that I've written this I see you've found the bug! Yay!

Wouldn't that call an error when trying to write said index?

Only if you're lucky. The C standard permits anything to happen when you try to read or write from an array at an invalid index. Typical actual behavior is that you corrupt a random part of your program's memory. You're lucky if your program crashes -- all sorts of other subtle behavior can be triggered by this error.

 

[cubanmissile]

char firstName[NAMELENGTH]

As soon as I saw this, my best two guesses for what's wrong with your code are:

• You forgot to account for the trailing null character: such an array can only contain a string of length NAMELENGTH-1.
• You aren't doing adequate bounds checking, so someplace you tried to write a string that was too long

Now, the quoted line above is not wrong, per se: it's a common C paradigm. But unfortunately, there are a good number of bugs and design flaws that are related to it.

fscanf(file, "%s %s %s %d %d", &class[j].firstName[0], &class[j].lastName[0], month, &class[j].day, &class[j].year);

Seeing this line makes me extremely suspicious that point 2 is happening. This code reads in strings from a file without any attempt to verify that the strings are not too long.

Additionally, you don't do any error checking on your file operations, which makes me mildly suspicious that there could have been an error you never noticed, and that's making things behave differently than you expect.




But now that I've written this I see you've found the bug! Yay!


Only if you're lucky. The C standard permits anything to happen when you try to read or write from an array at an invalid index. Typical actual behavior is that you corrupt a random part of your program's memory. You're lucky if your program crashes -- all sorts of other subtle behavior can be triggered by this error.

To answer your points, which are ALL valid and I hope others read this!

1. I did account for null character, assignment says name can be as long as 29 characters (guaranteed) so NAMELENGTH is 30.

2. You are CORRECT! I was calling my sort array using the end index as "students" instead of "students-1". Since I don't do out-of-bounds in my swap, since I assume sort is calling with correct bounds since I assume my sort is called with correct bounds, I wasn't able to free memory.


Thank you very much all for the help, now while debugging I have more things to keep in mind, which helped me find a circular loop in my sort function. :)

Thanks again, really!

 

[rcgldr]

The reason that free() got an error is that the compiler stores pointers and/or sizes just before and/or after any block of allocated memory, which free() uses to merge blocks of freed memory in the heap. Since your program used an index that was too large, it messed up the data that free() uses to merge freed memory, and that caused the reported error.