calloc free fails

[Zapeth]

Hello, I'm trying to read all content from a text file into a string. This is the code I'm using for it (I know there are other, maybe better ways to do it but I'd like to stick to this for now):

char* buffer;
std::string data;
FILE* fp = fopen("textfile.txt", "rb");
if (!fp)
{
	printf("Can't open file");
	return;
}

fseek(fp, 0L, SEEK_END);
lSize = ftell(fp);
rewind(fp);

// allocate memory for entire content
buffer = (char*)calloc(1, lSize+1);
if(!buffer)
{
	fclose(fp);
	printf("Failed to allocate memory");
	return;
}

// copy the file into the buffer
if(fread(buffer, lSize, 1, fp) != 1)
{
	fclose(fp);
	free(buffer);
	printf("Read operation from file failed");
	return;
}

fclose(fp);
data = buffer;
free(buffer);

So my problem is that I get an exception when I try to free the memory ->

0xC0000005: Access violation reading location 0x51366199

I even get the exception when I try to free it immediately after calloc() but I have no idea why this is.
And if I use buffer = (char*)malloc(lSize); I don't get any exceptions.

So I'm hoping someone can explain to me why this fails and what I'm doing wrong.

[kbw]

Is that part of a larger program? It sounds like you corrupted the heap earlier. Look for memory overruns and freeing free'd blocks.

[Zapeth]

It is part of a larger program but this code section is taken directly from it so buffer is not being modified in between.

So free() would throw an exception if the heap was corrupted at any earlier point in the program even if its not related to the memory block I'm trying to free?

edit: Also why does it work with malloc() then?

[kbw]

It is part of a larger program but this code section is taken directly from it so buffer is not being modified in between.

So free() would throw an exception if the heap was corrupted

No it wouldn't. Usually the program just crashes if you're lucky. If you're unlucky, it does seemingly random things.

[Zapeth]

It doesn't seem to occur on a random basis though. If I skip this part of code, the program runs fine and if I use malloc instead it runs fine.

But when I use calloc like here he gives an exception every time in the same line of the code (he crashes in Release mode and logs an exception in Debug mode).

[MikeyBoy]

How certain are you that the crash is occurring because of the free statement on line 34? Have you stepped through it with a debugger?

[Zapeth]

Of course I did. And every single time when I step over the free statement in Debug mode I get this in the output log (with different pointer locations each time of course):

First-chance exception at 0x77c6e3be in my_program.exe: 0xC0000005: Access violation reading location 0x1bf074d5

Btw I use Visual Studio 2010 and as I mentioned earlier if I use malloc instead of calloc I don't get any exceptions when stepping over free (or at any other point when the program is running).

[MikeyBoy]

Of course I did.

You'd be amazed how few people do. Even when advised to on here.

Is there any significant difference between the contents of buffer and data when you use malloc, and when you use calloc?

Is there any reason you're constrained to using pure C for this work, rather than using STL strings and new and delete?

[kbw]

That's agood point. data = buffer; assumes buffer is a null terminated sequence. But as you're reading a binary block that could contain zeros, you should do data.assign(buffer, lSize); However, that won't cause the crash.

I stand by my earlier advice; earlier heap corruption.

[Zapeth]

Content-wise there seems to be no difference between buffer and data (assuming you meant to compare them after the fread operation).

However if I use malloc, data ends up to be larger than the size I specified for allocation (lSize+1), to be exact lSize+1 = 1104 bytes while data.size() = 1112 bytes.

I'm not sure though if its not the same case with calloc since std::string data = buffer only copies the data up to the first 0 char (if I understood this correctly) and since calloc fills the allocated memory with 0 char's then the string would be the same size as buffer.

And yeah there is a reason I'm doing it this way but thats not the point of discussion here.

[kbw]

The reason data is larger with malloc than with calloc is because the buffer doesn't have any zeros.

calloc intialises the buffer with zeros, and since you read one less than the size of the buffer, it's always null terminated. But malloc doesn't touch the buffer, so the last character is random and clearly not zero.

As you're doing

data = buffer;

it's reading past the end of the buffer. So, I repeat, you should be using

data.assign(buffer, lSize);

But that isn't what's causing your crash.

[Disch]

Manual memory management is error prone.

Mixing C I/O (cstdio) with C++ functionality (std::string) is also error prone.

This entire routine can be simplified and made more safe by picking one language and sticking with it. Since you're using C++, I recommend using the C++ solutions rather than C's:

#include <fstream>
#include <sstream>

std::string loadFileIntoString(const char* filename)
{
    std::ifstream f(filename);
    if(!f.good())
    {
         // file couldn't be opened... handle error however you want here
    }
    std::stringstream ss;
    ss << f.rdbuf();
    return ss.str();
}

// then replace your code with:
std::string data = loadFileIntoString( "textfile.txt" );

malloc/calloc/free have no place in C++.



EDIT:

But yeah I don't see what would be causing the crash in that code you posted, so it may be coming from somewhere else in the program. Heap corruption is nasty business.

[JLBorges]

This would avoid copying a potentially large string:

#include <fstream>
#include <string>
#include <iterator>

std::string loadFileIntoString(const char* filename)
{
    std::ifstream f(filename);

    return { std::istreambuf_iterator<char>(f), std::istreambuf_iterator<char>() } ;
}

[Disch]

Nice.

[Zapeth]

Alright, I guess I'll be looking out for any potential memory heap corruption elsewhere in the code (I was really hoping to avoid that, urgh).

Anyway, thanks for all the replies so far.