[C/C++] fread problem *SOLVED*

Started by
8 comments, last by nullsquared 15 years, 9 months ago
I have some code for an XML parser that i am using for my game. I did not write the code myself, i got it from a friend who is helping me with my game. I have a problem with how it reads the file, it seems to overshoot the end of the file by about 10 characters. This is the read code (i wrote the code to strip comments ):

XMLNode *readXML(const char *file) {
    FILE *in;
    XMLNode *root;
    char *data;
    int nb;

    fopen_s(&in, file,"rb");
    if (!in)return 0;

    fseek(in,0,2);
    nb = ftell(in);   //### Problem seems to be here
    fseek(in,0,0);

    data = new char[nb];
    fread(data,1,nb,in);
    fclose(in);

    // Strip comments <!-- -->
    ...  //### This code isn't part of the problem (i hope!)
    nb = pos;
    data[nb] = '\0';

    root = new XMLNode("root");
    populateObj(root,data,nb);

    delete data;

    return root;
}




It seems to set the size of the file larger than what it is. The result is that after fread, data is:



<mech>
    <name>Test</name>
    <maxHealth>100</maxHealth>
    <img id=arm file=armImg.png/>
    <img id=armMask file=armMask.png/>
    <anim id=walkAnim file=walkAnim.png divX=50 divY=50/>
    <anim id=walkMask file=walkMask.png/>
    <anim id=jumpAnim file=jumpAnim.png/>
    <anim id=jumpMask file=jumpMask.png/>
</mech>ýýýý««««««««þîþîþîþ
I should point out that i am using Visual Studio on WinXP. Also, if i don't have the comments , i get a runtime error: "HEAP CORRUPTION DETECTED" which i traced to the code delete data; (that may be a result of my 'strip comments' code). EDIT: after further testing, i found that the problem is only on a certain XML file and i don't get the error if i remove comments from other files. To see the whole document, see here. If someone could help me out here that would be great, thanks. [Edited by - XTAL256 on July 1, 2008 3:47:17 AM]
[Window Detective] - Windows UI spy utility for programmers
Advertisement
Can you use C++ file I/O instead (much less error prone)? If so, here's how you can get the content of a text file much more simply.


std::string getFileAsString( const std::string& fileName ){	std::ifstream ifs( fileName.c_str() );	if ( ! ifs )	{		// Failed to open file! React to the error.		// ...	}	/// Get the content of the file into a string and return it to the caller.	const std::string fileContent( std::istreambuf_iterator( ifs ), std::istreambuf_iterator() );		return fileContent;}
Yeah, since i am using C++ i may as well change all of that code to use C++ I/O (it was originally written in C). Hopefully that will fix my problem, although i don't really see a problem with the current code anyway, so...
I'll get back to you on that.
[Window Detective] - Windows UI spy utility for programmers
Oh by the way ... (check the comments I added)

XMLNode *readXML(const char *file) {    FILE *in;    XMLNode *root;    char *data;    int nb;    fopen_s(&in, file,"rb");    if (!in)return 0;    fseek(in,0,2);    nb = ftell(in);     fseek(in,0,0);    data = new char[nb]; // <---- array too small, see comment below    fread(data,1,nb,in);    fclose(in);    // Strip comments <!-- -->    ...  //### This code isn't part of the problem (i hope!)    nb = pos;    data[nb] = '\0'; // <---- ERROR: Accessing (n + 1)th element in an array of n elements!!!!!!     root = new XMLNode("root");    populateObj(root,data,nb);    delete data;    return root;}
Regarding the heap corruption, the problem is with your delete call. When deleting an array, you must you the array version of delete:
delete[] data;

Better yet, use a container (or in this case a string maybe), and you won't have to worry about things like this.
ftell is not guaranteed to work except on binary files. Not a problem for you. fseek using SEEK_END is not guaranteed to work correctly with binary files.

Try checking the value of nb right now, then modify your code to read one byte at a time until the end of file is reached and compare the values. It's very likely they're different.
-- gekko
XMLNode *readXML(const char *file) {    std::ifstream file_stream(file, std::ios::binary);    std::istreambuf_iterator< char > begin(file_stream);    std::istreambuf_iterator< char > end;    std::vector< char > data(begin, end);    std::auto_ptr< XMLNode > root(new XMLNode("root"));    populateObj(root,data,nb);    return root.release();}

Σnigma
Quote:Original post by Red Ant
Oh by the way ... (check the comments I added)
*** Source Snippet Removed ***

Ah, yes you're right. Although the array isn't "too small", i just accessed beyond the last element.

Quote:Original post by Morrandir
delete[] data;

Again, another silly mistake (although that one wasn't mine[grin])

@gekko: i was a bit concerned with that bit of code since i wasn't sure if that would work on all systems. I had a look on the internet and found an example where it used the same code, but i still wasn't convinced that it was the standard way of getting the file length.

Well hopefully all those problems will be solved when i use C++ I/O.
thanks
[Window Detective] - Windows UI spy utility for programmers
Quote:Original post by Red Ant
Can you use C++ file I/O instead (much less error prone)? If so, here's how you can get the content of a text file much more simply.


// Oops! This is actually a function declaration.//const std::string fileContent( std::istreambuf_iterator( ifs ), std::istreambuf_iterator() );	// And then this would break.// return fileContent;// Using an unnamed temporary should work:return std::string(std::istreambuf_iterator(ifs), std::istreambuf_iterator());



But yes. To the OP, XML files aren't really supposed to be handled as binary files, and reading a whole file into a byte buffer is usually the wrong design approach anyway, for any kind of file. (If you really need everything in memory for performance reasons, then (a) are you sure? That really only matters with very large files normally and (b) you probably want some OS-specific solution for memory-mapped files.) Frankly, your friend is doing all kinds of dangerous and primitive stuff for no reason. I would recommend you get him a good reference on modern C++, so he can stop writing such abominations :) Here is a decent place to start.

Quote:Original post by Enigma
*** Source Snippet Removed ***
Σnigma


Why the auto_ptr? It'd only cause headaches if populateObj doesn't use a reference.

@ OP: For XML, any reason to not just use a full parser? Like, say, tinyxml? Then your above code would magically become:
TiXmlDocument doc(fileName);doc.LoadFile();TiXmlElement *root = doc.RootElement();

This topic is closed to new replies.

Advertisement