1

What's the problem with this code? It crashes every time.

One time it's a failed assertion "_ASSERTE(_CrtIsValidHeapPointer(pUserData));", other times it is just a "heap corrpuption" error.

Changing the buffer size affects this issue in some strange ways - sometimes it crashes on the "realloc", and other times on the "free".

I have debugged this code many times, and there is nothing abnormal regarding the pointers.

char buf[2000];
char *data = (char*)malloc(sizeof(buf));
unsigned int size = sizeof(buf);

for (unsigned int i = 0; i < 5; ++i)
{
 char *ptr = data + size;
 size += sizeof(buf);
 char *tmp = (char*)realloc(data, size);
 if (!tmp)
 {
  std::cout << "Oh no..";
  break;
 }
 data = tmp;
 memcpy(ptr, buf, sizeof(buf));
}

free(data);

Thanks!

Billy ONeal
  • 104,103
  • 58
  • 317
  • 552
Windindeed
  • 13
  • 1
  • 3
  • 1
    (Sorry for Phantom edit) The real question is: Why are you using realloc in C++? That's what `std::vector` is for. – Billy ONeal Jun 07 '10 at 21:07
  • 1
    In reality I'm writing a class.. memory is released in the destrucor, so no harm is ever possible. std::vector is just too heavy for this specific purpose. – Windindeed Jun 07 '10 at 21:15
  • 1
    what exactly do you see as "heavy" about `vector`? In reality, with even the slightest care it'll almost always be considerably *faster* than this (quite possibly with smaller code). – Jerry Coffin Jun 07 '10 at 21:36
  • @Jerry Coffin. I have the same requirement. Suppose you have a symmetric matrix to store and you want to access the diagonal values. In vector language it would be something like vector> matrix. The vector overhead under x64 in Windows with 1bit allignment is 40 bytes. If you implement your own container just for storage, say name it SimpleArray, it could be of a overhead as small as 8 bytes with no performance overhead. Consider there are millions of these matrices with degree of 100 or so. The inner vector better be replaced with SimpleArray. The ram benefit is prominent. – Glenn Yu Jan 25 '13 at 09:08
  • 1
    @GlennYu: Yes, vectors of vectors of vectors is rarely a good idea (unless you really need to store "ragged" arrays throughout). For a case like this, building your own 2D or [3D array](http://stackoverflow.com/a/2216055/179910) makes sense -- but it should still usually use `vector` for the underlying storage (that example is old enough that it doesn't, but it really should). – Jerry Coffin Jan 25 '13 at 14:27
  • @JerryCoffin, Thanks for the explanation. I have learned from it. – Glenn Yu Jan 25 '13 at 14:57
  • Can you change the title to "copying to the old location after reallocating memory seems to cause a heap corruption"? Because that's the problem, calculating ptr much too early. – gnasher729 May 19 '14 at 09:45

3 Answers3

1

You're trashing the heap. realloc can freely choose to return you memory from an entirely different location as it reallocates, and this is invalidating your ptr. Set ptr after reallocating.

Matti Virkkunen
  • 63,558
  • 9
  • 127
  • 159
  • Yep.. After writing the post exactly that caught my eye.. I guess all that's required to get an answer is to write a post.. especially if you're debugging your application for nearly 4 hours to find a problem. :D Thanks! – Windindeed Jun 07 '10 at 21:11
0

On the second iteration of the loop here are the values

  • data points to a buffer of size sizeof(buf)
  • size has a value of sizeof(buf)

Given these values the value of ptr is that it points past the end of the buffer allocated into data. This is memory not owned by the process and the following memcpy operation writes to this and corrupts memory.

JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
0
char *ptr = data + size;
char *tmp = (char*)realloc(data, size);
memcpy(ptr, buf, sizeof(buf));

The call to realloc() here can potentially free the old buffer, before returning the new one.

John Millikin
  • 197,344
  • 39
  • 212
  • 226