fread causes program to terminate

[emilo35]

Okay, so what I'm trying to do is to write a program that reads several TGA images and bunch them together into one bin-file for later use. I got this code from NeHe OpenGL, Tutorial 24, and it is what I use to load my images:

bool loadTGA(char *path, TextureImage *texture)
{
	GLubyte		TGAheader[12]={0,0,2,0,0,0,0,0,0,0,0,0};
	GLubyte		TGAcompare[12];	
	GLubyte		header[6];
	GLuint		bytesPerPixel;
	GLuint		imageSize;	
	GLuint		temp;		
	GLuint		type=GL_RGBA;
	cout << "Attempting to open '" << path << "'..\n";
	FILE file  = fopen(path, "rb");
	if(	file==NULL ||
		fread(TGAcompare,1,sizeof(TGAcompare),file)!=sizeof(TGAcompare) ||
		memcmp(TGAheader,TGAcompare,sizeof(TGAheader))!=0				||
		fread(header,1,sizeof(header),file)!=sizeof(header))
	{
		if (file == NULL){
			cout << "file == NULL!\n";
			return false;}	
		else
		{
			fclose(file);
			return false;
		}
	}
	texture->width  = header[1] * 256 + header[0];
	texture->height = header[3] * 256 + header[2];
    
 	if(	texture->width	<=0	||
		texture->height	<=0	||
		(header[4]!=24 && header[4]!=32))
	{
		fclose(file);
		return false;
	}
	texture->bpp	= header[4];	
	bytesPerPixel	= texture->bpp/8;
	imageSize		= texture->width*texture->height*bytesPerPixel;

	texture->imageData=(GLubyte *)malloc(imageSize);

	if(	texture->imageData==NULL ||
		fread(texture->imageData, 1, imageSize, file)!=imageSize)
	{
		if(texture->imageData!=NULL)
			free(texture->imageData);

		fclose(file);
		return false;	
	}
	for(GLuint i=0; i<GLuint(imageSize); i+=bytesPerPixel)
	{		
		temp=texture->imageData[i];
		texture->imageData[i] = texture->imageData[i + 2];
		texture->imageData[i + 2] = temp;
	}
	fclose (file);
	return true;	
}

Also, if you're curious about the anatomy of that TextureImage..

typedef struct
{
	GLubyte	*imageData;
	GLuint	bpp; //Bits per pixel
	GLuint	width;
	GLuint	height;
	GLuint	texID;
} TextureImage;

Everything works fine when loading single images. However, when I called loadTGA from a loop, using with different filenames my program suddenly started to crash when reading the third image. So it works for image 1 and 2, but crashes at 3..

So I started flushing out couts everywhere until I could narrow it down to the if-statement starting at line 12. So I broke it apart and found out it was the line fread(TGAcompare,1,sizeof(TGAcompare),file) that caused my program to terminate. I have no chance of checking what it returns, since the program seems to terminate during the execution of fread. I receive no warnings/errors during compile (using Dev-Cpp, WinXP)

Some things I've checked:
- file is open and exist
- it is not empty, and has not reached EOF
- it's not because of a damaged file, since I tried reading img1.tga->img2.tga->img1.tga instead of img1.tga->img2.tga->img3.tga. Still crash at third image.

I'm all out of ideas, and I'm quite frustrated. Does anyone have a clue to what might cause this to happen?

Thanks a lot for your time!

Emil Olofsson

[mitrmkar]

I receive no warnings/errors during compile (using Dev-Cpp, WinXP)

That is too hard to believe, given that you have there ..

FILE file  = fopen(path, "rb");

So, could you verify that you are actually posting the relevant code?

[emilo35]

Yeah.. right.. I cut down the code to what I thought was the original code before I started debugging it. It really is a mess now, but I understand that you want to see my actual code I'm working with.

This is the actual code I have at the moment, and it compiles and gives the same errors as described above:

bool loadTGA(char *path, TextureImage *texture)
{
	GLubyte		TGAheader[12]={0,0,2,0,0,0,0,0,0,0,0,0};
	GLubyte		TGAcompare[12];	
	GLubyte		header[6];
	GLuint		bytesPerPixel;
	GLuint		imageSize;
	GLuint		temp;
	GLuint		type=GL_RGBA;
	cout << "Attempting to open '" << path << "'..\n";
	FILE *file;	
	file = fopen(path, "rb");
	cout << " ..opened..\n";
	cout.flush();
	if (ferror (file)) {
		cout << "FERROR!\n";
		cout.flush();
		fclose(file);
		return false;
	} 
	else 
		cout << "NO FERROR\n";
	if (file==NULL)
	{
		cout << "File==NULL!\n";
		fclose(file);
		return false;
	} 
	else
		cout << "FILE NOT EMPTY\n";
	cout << "sizeof(TGAcompare) = " << sizeof(TGAcompare) << "\n";
	cout << "feof(file) = " << feof(file) << "\n";
	
	size_t t1 = fread(TGAcompare,1,sizeof(TGAcompare),file);
	
	cout << "fread(TGAcompare,1,sizeof(TGAcompare),file) = " << t1 << endl;
	cout << "sizeof(TGAcompare) = " << sizeof(TGAcompare) << "\n";
	if (t1 != sizeof(TGAcompare))
	{
		cout << "t1 != sizeof(TGAcompare)!\n";
		fclose(file);
		return false;
	}
	t1 = memcmp(TGAheader,TGAcompare,sizeof(TGAheader));
	cout << "memcmp(TGAheader,TGAcompare,sizeof(TGAheader)) = " << t1 << "\n";
	if (t1 != 0)
	{
		cout << "memcmp(TGAheader,TGAcompare,sizeof(TGAheader)) != 0..\n";
		fclose(file);
		return false;
	}
	t1 = fread(header,1,sizeof(header),file);
	cout << "fread(header,1,sizeof(header),file) = " << t1 << "\n";
	cout << "sizeof(header) = " << sizeof(header) << "\n";
	if (t1 != sizeof(header))
	{
		cout << "t1 != header. Quitting....\n";
		fclose(file);
		return false;
	}
	/*if(	file==NULL ||
		fread(TGAcompare,1,sizeof(TGAcompare),file)!=sizeof(TGAcompare) ||
		memcmp(TGAheader,TGAcompare,sizeof(TGAheader))!=0				||
		fread(header,1,sizeof(header),file)!=sizeof(header))
	{
		if (file == NULL){
			cout << "0.1 ";
			return false;}
		else
		{
			cout << "0.2 ";
			fclose(file);
			return false;
		}
	}*/
	cout << "1 ";
	texture->width  = header[1] * 256 + header[0];
	texture->height = header[3] * 256 + header[2];
    
 	if(	texture->width	<=0	||
		texture->height	<=0	||	
		(header[4]!=24 && header[4]!=32))
	{
		fclose(file);
		return false;
	}
	cout << "2 ";
	texture->bpp	= header[4];
	bytesPerPixel	= texture->bpp/8;
	imageSize		= texture->width*texture->height*bytesPerPixel;

	texture->imageData=(GLubyte *)malloc(imageSize);

	if(	texture->imageData==NULL ||	
		fread(texture->imageData, 1, imageSize, file)!=imageSize)
	{
		if(texture->imageData!=NULL)
			free(texture->imageData);

		fclose(file);
		return false;
	}
	cout << "3 ";
	for(GLuint i=0; i<GLuint(imageSize); i+=bytesPerPixel)
	{
		temp=texture->imageData[i];
		texture->imageData[i] = texture->imageData[i + 2];
		texture->imageData[i + 2] = temp;
		//cout << ".";
	}
	cout << "- 4 -\n";
	rewind(file);
	fclose (file);
	free(TGAheader);
	free(TGAcompare);
	free(header);
	cout << "\t..done!" << endl;
	cin.get();
	return true;	
}

[mitrmkar]

First thing to check in all of your code, how you are handling memory allocations/deallocations.

Now you are trying to free() variables that are on stack, that is ..

GLubyte TGAheader[12]={0,0,2,0,0,0,0,0,0,0,0,0};
GLubyte TGAcompare[12];
GLubyte header[6];
...
// Simply disastrous ...
free(TGAheader);
free(TGAcompare);
free(header);

You only free() what you have malloc()/calloc() ed, i.e. dynamic memory, nothing else.

PS. Since this is C++, you might be better of with new/new[] and delete/delete[] , respectively, if there's a need to use dynamic memory.

[emilo35]

Well, those were desperate attempts to eliminate the possibility of a memory leak being the source of the problem. It didn't really make any difference, but they were left hanging. Some guy out there on some forum on the web had a similar problem, and it was solved because he found a memory leak that forced his program to eventually terminate. Some other guy was using the enhanced modes for fread (like r+), but never used rewind/fseek etc so I tested that too. And then there was a lot of guys who tried to read bin files but only using r mode or similar, but that's not my case either.

[mitrmkar]

>> Well, those were desperate attempts to ..
Try to keep cool and go through the code making sure that you don't mess with the memory anywhere.

As to the original crash with fread() , there seems to be a malloc() , involved. Regarding that, make sure that you are allocating a reasonable memory block ( imageSize > 0 ) and that the malloc() actually succeeds.

[emilo35]

This is what happens at line 92. imageSize has a value of about 16000 in those cases that work, which seems reasonable since my images are about 16kb. I also made sure to use free(texture->imageData); every loop after I'm done with the data. Also, I think my RAM should be able to hold a little more than 32kb without making a fuzz about it, if there is a memory leak. Right? (Also malloc returns no NULL pointers, so it works)

What about collisions from other open files simultaneously? Keeping in mind that my program is running a pretty complex series of reads/writes and all. When running this particular operation, my program opens a *.txt file containing paths (like image1.tga, image2.tga etc) with an ifstream. It reads one path at a time, sending it to loadTGA. Also, I keep an oftream open where I put my TextureImages after loadTGA has done its job. That should be possible to do, right? After one run my destination file is about 32 kb, which pretty much equals two successfully processed images.

[Salem]

> size_t t1 = fread(TGAcompare,1,sizeof(TGAcompare),file);
Although it looks pretty, this isn't the way to read a binary file.

The compiler is free to add padding and alignment to the struct, which the external file format will know nothing about. So what you actually read will be skewed compared to what the file format actually means.

struct header {
    short int type;
    // 2 bytes of padding here
    long int length;
};

If you read this in, believing you're reading 6 bytes, then you're wrong on several counts
- you've read 8 bytes
- 2 bytes of length wound up in the dead space
- the other two bytes of length are in the wrong half of the word
- the remainder of the length contains 2 bytes of the NEXT data element.

Even if you manage to use pragma(pack) or its equivalents, no amount of compiler magic will fix the endian problem if your machine has a different byte order to the file format.

[emilo35]

Thanks for enlightening me on that point Salem. I surely have to take a closer look at this pack stuff. It really might come in handy some day!

Now back to my original problem; I solved it! I had actually gone to bed, when suddenly I got a flash of impulse to get up and try out one more thing. The problem actually was in the call to loadTGA. There I use a function that parses a string (the path from the *.txt file) to char*.

This is actually what I changed:

int tgalib(char *inP, char *outP) {
	TextureImage texture = *new TextureImage;
	string inStr;
	int images;
	char* path;
	ifstream ifs (inP);
	ofstream ofs (outP, ios::binary);
	getline (ifs,inStr);
	images = stringToInt(inStr);
	ofs.write((char *)&images, sizeof(images));
	for (int i = 0; i < images; i++)
	{
		getline (ifs,inStr);
		path = stringToChar(inStr); // Added this
		inStr.clear(); // and this
		// if (!loadTGA(stringToChar(inStr), &texture)) This was my previous call
		if (!loadTGA(path, &texture))
		{
			ofs.close();
			ifs.close();
			delete[] path;
			return 1; //Error: Could not load TGA
		}
		delete[] path;
		ofs.write((char *)&texture.texID, sizeof(texture.texID));
		ofs.write((char *)&texture.width, sizeof(texture.width));
		ofs.write((char *)&texture.height, sizeof(texture.height));
		ofs.write((char *)&texture.bpp, sizeof(texture.bpp));
		GLuint imageSize = texture.width*texture.height*(texture.bpp/8); //size in bytes
		ofs.write((char *)texture.imageData, imageSize);
		free(texture.imageData);
	}
	delete &texture;
	ofs.close();
	ifs.close();
	return 0;	
}

Thanks for all your time and effort guys!