Using delete [] (heap corruption) when implementing the + = operator
I've been trying to figure this out for hours and I'm at my end. I would be very grateful if someone can tell me when I am wrong.
I wrote a simple class to emulate basic string functionality. The classes include a pointer to a data strong> character (which points to a dynamically created char array) and an integer strSize (which contains the length of the string, without a terminator.)
Since I am using new and delete , I have applied the copy constructor and destructor. My problem occurs when I try to implement the + = operator . The LHS object builds the newline correctly - I can even print it out using cout, but the problem comes up when I try to free the data pointer in the destructor: I get "Heap corruption detected after normal block" at the memory address pointed to by the data array which the destructor is trying to free.
Here's my complete class and test program:
#include <iostream>
using namespace std;
// Class to emulate string
class Str {
public:
// Default constructor
Str(): data(0), strSize(0) { }
// Constructor from string literal
Str(const char* cp) {
data = new char[strlen(cp) + 1];
char *p = data;
const char* q = cp;
while (*q)
*p++ = *q++;
*p = '\0';
strSize = strlen(cp);
}
Str& operator+=(const Str& rhs) {
// create new dynamic memory to hold concatenated string
char* str = new char[strSize + rhs.strSize + 1];
char* p = str; // new data
char* i = data; // old data
const char* q = rhs.data; // data to append
// append old string to new string in new dynamic memory
while (*p++ = *i++) ;
p--;
while (*p++ = *q++) ;
*p = '\0';
// assign new values to data and strSize
delete[] data;
data = str;
strSize += rhs.strSize;
return *this;
}
// Copy constructor
Str(const Str& s)
{
data = new char[s.strSize + 1];
char *p = data;
char *q = s.data;
while (*q)
*p++ = *q++;
*p = '\0';
strSize = s.strSize;
}
// destructor
~Str() { delete[] data; }
const char& operator[](int i) const { return data[i]; }
int size() const { return strSize; }
private:
char *data;
int strSize;
};
ostream& operator<<(ostream& os, const Str& s)
{
for (int i = 0; i != s.size(); ++i)
os << s[i];
return os;
}
// Test constructor, copy constructor, and += operator
int main()
{
Str s = "hello"; // destructor for s works ok
Str x = s; // destructor for x works ok
s += "world!"; // destructor for s gives error
cout << s << endl;
cout << x << endl;
return 0;
}
EDIT : Accelerated C ++ 12-1 issue.
a source to share
There are a bunch of good answers already here, but it's worth including Valgrind as a tool to solve just this kind of problem.If you have access to a * nix window, Valgrind's tools can be a real lifesaver.
Just to show you, this is what I got when compiling and running your program through it:
% g ++ -g -o test test.cpp % valgrind ./test == 2293 == Memcheck, a memory error detector == 2293 == Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. == 2293 == Using Valgrind-3.5.0-Debian and LibVEX; rerun with -h for copyright info == 2293 == Command: ./test == 2293 == == 2293 == Invalid write of size 1 == 2293 == at 0x8048A9A: Str :: operator + = (Str const &) (test.cpp: 36) == 2293 == by 0x8048882: main (test.cpp: 82) == 2293 == Address 0x42bc0dc is 0 bytes after a block of size 12 alloc'd == 2293 == at 0x4025024: operator new [] (unsigned int) (vg_replace_malloc.c: 258) == 2293 == by 0x8048A35: Str :: operator + = (Str const &) (test.cpp: 26) == 2293 == by 0x8048882: main (test.cpp: 82) == 2293 == helloworld! hello == 2293 == == 2293 == HEAP SUMMARY: == 2293 == in use at exit: 0 bytes in 0 blocks == 2293 == total heap usage: 4 allocs, 4 frees, 31 bytes allocated == 2293 == == 2293 == All heap blocks were freed - no leaks are possible == 2293 == == 2293 == For counts of detected and suppressed errors, rerun with: -v == 2293 == ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 17 from 6) %
You can see that it brought up the lines that the other answers are listed here (near line 36).
a source to share
while (*p++ = *i++) ; // the last iteration is when i is one past the end
// i is two past the end here -- you checked for a 0, found it, then incremented past it
p--; //here you corrected for this
while (*p++ = *q++) ;// the last iteration is when p and q are one past the end
// p and q are two past the end here
// but you didn't insert a correction here
*p = '\0'; // this write is in unallocated memory
Use an idiom similar to what you used in the copy constructor:
while (*i) *p++ = *i++; //in these loops, you only increment if *i was nonzero
while (*q) *p++ = *q++;
*p = '\0'
a source to share
You already have two answers pointing to a specific error that is causing you to delete the heap. Assuming this is homework or some other form of exercise (otherwise we'll all yell at you for writing your own string class), here are a few more things to chew on for you:
- If you feel the need to annotate your code, consider making it more expressive .
For example,char* p = str; // new data
you can simply write insteadchar* new_data = str;
.
Instead of//do frgl
being followed by a piece of code, you can simply writedo_frgl();
. If the function is inline, it makes no difference to the resulting code, but it has a lot of value to readers of the code. - Everyone, including your header, gets everything from the namespace
std
dumped into the global namespace . This is not a good idea. I wouldn't include your headline like the plague. - Your constructors must initialize the members of the class in initialization lists .
- The constructor
Str::Str(const char*)
callsstd::strlen()
twice for the same string.
Application code should be as fast as possible , library code on the other hand, where you don't know which application it ends up with, should be as fast as possible . You are writing library code. - Can a member function
size()
return a negative value ? If not, why is it a signed integer? - What happens for this code
Str s1, s2; s1=s2
:? - And how about this:
Str str("abc"); std::cout<<str[1];
(If anyone who comes across this might think of more hints, feel free to expand on this.)
a source to share