Problem using delete[] (Heap corruption) when implementing operator+= (C++)

Question

I've been trying to figure this out for hours now, and I'm at my wit's end. I would surely appreciate it if someone could tell me when I'm doing wrong.

I have written a simple class to emulate basic functionality of strings. The class's members include a character pointer data (which points to a dynamically created char array) and an integer strSize (which holds the length of the string, sans terminator.)

Since I'm using new and delete, I've implemented the copy constructor and destructor. My problem occurs when I try to implement the operator+=. The LHS object builds the new string correctly - I can even print it using cout - but the problem comes when I try to deallocate the data pointer in the destructor: I get a "Heap Corruption Detected after normal block" at the memory address pointed to by the data array the destructor is trying to deallocate.

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++ problem 12-1.

Answer 1

Following chunk of code makes p pointed beside the array.

while (*p++ = *q++) ;
*p = '\0';

Better (and safe) solution you have used in copy constructor:

while (*q)
    *p++ = *q++;
*p = '\0';

Answer 2

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'

Answer 3

You already have two answers pointing at the specific error that made you trash the heap. Assuming this is homework or some other form of exercise (otherwise we'd all be yelling at you for writing your own string class), here's a few more things to chew on for you:

• If you feel the need to annotate the code, consider making it more expressive.
  For example, instead of char* p = str; // new data, you could just write char* new_data = str;.
  Instead of //do frgl, followed by a chunk of code, you could just write do_frgl();. If the function is inlined, it makes no difference for the resulting code, but a lot of difference to readers of the code.
• Everyone including your header gets everything from namespace std dumped into the global namespace. That's not a good idea at all. I'd avoid including your header like the plague.
• Your constructors should initialize their class' members in initializer lists.
• Your Str::Str(const char*) constructor calls std::strlen() twice for the same string.
  Application code should be as fast as needed, library code, on the other hand, where you don't know which application it ends in, should be as fast as possible. You're writing library code.
• Could the size() member function ever return a negative value? If not, why is it a signed integer?
• What would happen for this code: Str s1, s2; s1=s2?
• And what about this: Str str("abc"); std::cout<<str[1];

(If anyone coming across this can think of more hints, feel free to expand this.)