Memory leak error?

Question

For a simple assignment to do with dynamic memory and copy constructors, my prof has assigned us a simple assignment, but I get an error during the second time my delete [] happens.

header file:

class Stream {
    int len;
    char *hold;
    char* newmem(int);
public:
    Stream ();
    Stream (int);
    Stream(const char *);
    ~Stream ( );
    void operator=(const Stream &);
    Stream(const Stream &);
    friend void show(Stream);
    void operator<<(const char*);
};

it should be fairly simple. here is my code:

#include <iostream>
#include <new>
#include <cstring>
using namespace std;
#include "stream.h"

char* Stream::newmem(int x) {
    char * tmp;
    try {
        tmp = new char[x];
    }
    catch(std::bad_alloc) {
        tmp = NULL;
    }
    if(tmp)
        cout << "newmem:  " << (void *) tmp << endl;
    return tmp;
}

Stream::Stream ( ) {
    len = 1000;
    hold = newmem(len);
    if (hold)
        strcpy (hold, "");
}

Stream::Stream(int n) {
    len = n;
    hold = newmem(len);
    if (hold)
        strcpy (hold,"");
}

Stream::Stream(const char * dat) {
    len = strlen(dat) +1;
    hold = newmem(len);
    if (hold)
        strcpy(hold,dat);
}

Stream::Stream(const Stream &from) {
    cout << "in the copy constructor, allocating new memory..." << endl;
    cout << "original pointer address is: " << (void *) from.hold << endl;
    cin.get( );

    len=from.len;
    hold=newmem(len +1);
    cout << "new pointer address is: " << (void *) hold << endl;
    cin.get( );

    if(hold)
        strcpy (hold,from.hold);
}

Stream::~Stream ( ) {
    cout << "destruct:  " << (void *) hold << endl;
    cin.get( );
    if (hold)
        delete [] hold;
}

void Stream::operator= (const Stream &from) {
    if(hold)
        delete [ ] hold;
    len = from.len;
    hold=newmem(len +1);
    if (hold)
        strcpy(hold,from.hold);
}

void show (Stream prt) {
    cout << "String is: " << prt.hold << endl << "Length is: " << prt.len << endl;
}

void Stream::operator<< (const char *data) {
    int dlen = strlen(data);
    for (int i=0 ; i<=len && i<=dlen ; i++) {
        hold[i] = data[i];
    }
}

int main( ) {
   char data[ ] = "Growing up it all seems so one-sided;"
                  "Opinions all provided;"
                  "The future pre-decided;"
                  "Detached and subdivided;"
                  "In the mass production zone!"
                  "-Neil Peart- \"Subdivisions\"";

   Stream x1, x2(25), x3;
   x1 << data;
   x2 << data;
   show(x1);
   show(x2);

   x3 = x2;
   show(x3);

   return 0;
}

and my output / error:

in the copy constructor, allocating new memory...
original pointer address is: 0x804c008

new pointer address is: 0x804c808

String is: Growing up it all seems so one-sided;Opinions all provided;The future pre-decided;Detached and subdivided;In the mass production zone!-Neil Peart-Subdivisions"
Length is: 1000
destruct:  0x804c808


in the copy constructor, allocating new memory...
original pointer address is: 0x804c3f8

new pointer address is: 0x804c808

String is: Growing up it all seems so
Length is: 25
destruct:  0x804c808

*** glibc detected *** a.out: free(): invalid pointer: 0x0804c808 ***

Answer 1

First a little self-help advice: The most important tool for catching memory access errors is valgrind. Run it on your program, and you'll get a warning every time you try to access unallocated or uninitialized memory. It's no substitute for knowledge, but the next best thing.

While I get different output than you get, errors seem to interact here:

1. The operator<< has an off-by-one error in the range check. It writes one byte too much (hold[len]).
2. operator<< does never write the terminating null byte. Both errors are invoked by x2 << data.
3. When the copy constructor tries to copy the string from x2, strcpy finds no terminating null byte and both reads right off the end of x2.hold and writes past the end of x3.hold. The latter has the potential for unbounded corruption and probably caused your error.

Whenever you deal with C strings, make very, very sure to get termination right. The fixed version is:

void Stream::operator<< (const char *data) {
    int dlen = strlen(data);
    hold[len-1] = 0;
    for (int i=0 ; i < len-1 && i <= dlen ; i++) {
         hold[i] = data[i];
    }
}

Or, using the std library:

void Stream::operator<< (const char *data) {
    strncpy(hold, data, len);
    hold[len-1] = 0;
}

Answer 2

The for loop in the operator<< has two off-by-one errors:

for (int i=0 ; i<=len

allows i==len, but the only valid indices of hold are 0..(len-1). So, you can write one off the end.

Secondly, as thiton pointed out, it doesn't copy the \0 terminator even if there is space.

---

A correct implementation might look like:

void Stream::operator<< (const char *data) {
    int source_len = strlen(data);
    int copy_len = min(source_len, len-1); // allow for terminator

    for (int i=0; i<copy_len; i++) {
        hold[i] = data[i];
    }
    hold[copy_len] = '\0';
}

although it'd be better practise to simply use strncpy.

---

Note that the idiom of using half-open (or one-past-the-end) ranges is standard not only in direct array indexing, but also with C++ iterators. So, you should always expect to see

for (i=0; i<n; ++i) {

or

for (i = begin; i != end; ++i) {

and should generally treat closed-range loops like yours as a smell that warrants further investigation.