1

I am having to work with someone else's code, so I am not entirely in control over what I can change but I am wondering why I am getting this error and would appreciate any suggestions. This code is in the part that I can't change but I need to make sure the part I wrote (StringRef class) works.

The error it gives me is 'Heap block at X modified at Y past requested size of 28'. If I change the existing code which is using realloc to malloc it kicks the can down the road a bit and loads a few more values into the array. Here are the lines in question. I can't include all the code as it is too extensive. Is this enough info to diagnose what I am doing wrong?

struct StringList
{
    StringRef *elements;
    unsigned int count;
};

.....

// Append the given StringRef to the list.
bool StringListAppend(StringList& self, StringRef p_string)
{

    StringRef *t_new_elements;
    t_new_elements = (StringRef *)realloc(self.elements, (self.count + 1) * sizeof(StringRef *));
    if (t_new_elements == NULL)
        return false;

    self.elements = t_new_elements;
    std::cout<<self.count<<"\n";
    // Initialize and assign the new element.
    StringInitialize(self.elements[self.count]);
    if (!StringAssign(self.elements[self.count], p_string))
        return false;

    // We've successfully added the element, so bump the count.
    self.count += 1;

    return true;
}

vs

StringRef *t_new_elements;
t_new_elements = (StringRef *)malloc((self.count + 1) * sizeof(StringRef *));

for the line with realloc averts the problem a little further.

Jonathon Reinhart
  • 132,704
  • 33
  • 254
  • 328
GenericJam
  • 2,915
  • 5
  • 31
  • 33
  • 3
    Do you understand the difference between `realloc` and `malloc`? If not you should probably do so first. `realloc` will copy the old contents of the allocation, where `malloc` will not. Changing the code to use `malloc` is totally leaking the previous allocation, and also forgetting about all of the previous contents of `self.elements`. Don't "kick the can down the road". Figure out the real bug (buffer overrun) and *fix it*. – Jonathon Reinhart Mar 03 '14 at 23:45
  • 4
    The only two things about this code that qualify as C++ are using `std::cout` and a reference. I feel bad for you if someone's calling this C++. – chris Mar 03 '14 at 23:48
  • I do 'basically' understand the difference between them. I am just trying to alter the existing code enough that the code I wrote can run or not run. I am a little more familiar (but rusty) regarding C++. I know some people like to use either/or/both when coding in C++ (as the author of this code is doing). I don't know how to track down the buffer overrun apart from what I am already doing which is changing out factors that could be the problem and trying to track down the source of the bug. – GenericJam Mar 03 '14 at 23:59
  • `realloc` can be used to 1) allocate memory blocks 2) resize member blocks and 3) freeing memory blocks. For this reason it is very comfortable to use just `realloc` alone in your code but beware! On some platforms `realloc`ing an existing block to zero size `free`s the block while on some other platforms it just returns a zero sized block that must be `free`d later. – pasztorpisti Mar 04 '14 at 00:00
  • @pasztorpisti - The author of the code is just using it to resize the array so it shouldn't have that side effect here. – GenericJam Mar 04 '14 at 00:02
  • @GenericJam sure, I just motivated the author to use `realloc` only and to read the platform docs to avoid the leak, I've just found the `realloc` version that is different from the others with zero sized `realloc`: https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man3/realloc.3.html – pasztorpisti Mar 04 '14 at 00:06
  • @pasztorpisti - `realloc(ptr, 0)` *ALWAYS* deallocates `ptr` and returns a pointer to a block of zero size in a Standard conforming environment. – D.Shawley Mar 04 '14 at 00:38
  • @GenericJam - are the members of `StringList` initialized before calling `StringListAppend`? – D.Shawley Mar 04 '14 at 00:39
  • @D.Shawley ios is a popular environment and if you port existing code there you will be surprised. We fixed a few `realloc` related leaks there. Unfortunately "standard" is often just a dream in case of several APIs in the low level world sometimes even in case of popular apis like berkeley sockets... – pasztorpisti Mar 04 '14 at 00:42
  • @D.Shawley - No. That is the intent of the code I guess is to initialize and append them to the StringList. It's not really the way I would have done it but I'm just working with what is given to me. My problem was solved by the observation of Matt McNabb regarding an errant asterisk. – GenericJam Mar 04 '14 at 01:21

2 Answers2

2

Lets say you wish to allocate some memory. You should use malloc (stores memory on heap uninitialized) or calloc(stores initializes all elements to 0). Explained more Here!

Realloc extends the length of your allocated memory. So you need to allocate some before you can extend it. (dont neeed to, but it is good coding practice to do so) Realloc

I would suggest looking up more on memory allocation because abusing it can greatly reduce efficiency and must be treated with precaution, like: you should always free memory that you have allocated at the end of your program!

Community
  • 1
  • 1
Shrey
  • 671
  • 7
  • 14
  • While being perfectly valid and possibly useful to the question poster, it doesn't attempt to answer the question posted. Answers need to be constrained to that which actually attempts to answer the question. – mah Mar 04 '14 at 00:13
  • haha no @mah is right to say that! I didnt really answer the question. What i wished to do was to direct OP down a path which will be more useful later on as it is easy to change code now rather than 1000 lines into his program...especially when dealing with memory allocation. – Shrey Mar 04 '14 at 00:27
  • 1
    @πάνταῥεῖ I'm not picky, I'm aware of what the site's rules are -- and I'm aware that it would not be a surprise that after some number of people flag his post as not being an answer it will be deleted (perhaps by a moderator or perhaps automatically, I'm not sure how that works). Since Scarecrow is relatively new to the site, I decided to give him a gentle nudge since that's nicer than simply seeing your answer disappear with no explanation. – mah Mar 04 '14 at 00:31
  • Note that you don't actually need to allocate memory before passing it to `realloc`. For example, `realloc(NULL, sz)` is equivalent to `malloc(sz)`. – D.Shawley Mar 04 '14 at 00:35
  • yes that is why in brackets i wrote "dont neeed to, but it is good coding practice to do so" :) – Shrey Mar 04 '14 at 00:40
2

sizeof(StringRef *) should be sizeof(StringRef)

You could avoid this error by using this idiom:

ptr = realloc(old_ptr, n_elements * sizeof *ptr);

when the number of bytes to allocate is determined based on the type of the variable that holds the returned pointer; it doesn't require you to repeat anything and thereby introduce a discrepancy.

As to why changing realloc to malloc slightly moves the point at which the program crashes... undefined behaviour is undefined :)

M.M
  • 138,810
  • 21
  • 208
  • 365