How to use delete[] in destructor

Question

I have the following files for implementing a Bucket class. However I can't destroy the _str member in the destructor of the Buckets produced by the operator+. The error that I get is:

heap corruption detected after normal block...CRT detected that the application wrote to memory after end of heap buffer

The problem always occurs after the addition of the two Bucket objects, however I've checked during the debugging that the produced string is correct as well as the length.

Also, how can I access the _len attribute in the operator>> function in order to assign a new value?

Header File:

class Bucket
{
    friend ostream& operator<<(ostream& os, const Bucket& c);
    friend istream& operator>>(istream& is, const Bucket& c);
public:
    Bucket();
    Bucket(const String& str);
    Bucket(const char* str);
    ~Bucket();
    const Bucket operator+(const Bucket& s) const;
    const Bucket operator+(const char* s) const;
    const Bucket& operator=(const char* c);
    const bool operator==(const char* str);
private:
    char* _str;
    int _len;
};

Source File:

Bucket::Bucket() {
    _len = 0;
    _str = new char[_len];
}

Bucket::Bucket(const Bucket& myBucket) {
    _len = myBucket._len;
    _str = new char[_len + 1];
    for (int i = 0; i < _len; i++)
        _str[i] = myBucket._str[i];
    _str[_len] = '\0';
}

Bucket::Bucket(const char* str) {
    _len = 0;
    for (int i = 0; str[i] != '\0'; i++) _len++;
    
    _str = new char[_len + 1];
    for (int i = 0; i < _len; i++)
        _str[i] = str[i];
    _str[_len] = '\0';
}

Bucket::~Bucket() {
    if (_len > 0) delete[] _str;
}

const Bucket Bucket::operator+(const Bucket& myBucket) const {
    String tempBucket;
    tempBucket._str = strcat(this->_str, myBucket._str);
    int len = 0;
    
    while (tempBucket._str[len] != '\0') len++;
    tempBucket._str[len] = '\0';
    tempBucket._len = len;
    return tempBucket;
}

const Bucket Bucket::operator+(const char* str) const {
    Bucket tempBucket;
    
    int len = 0;

    tempBucket._len = this->_len;

    for (int i = 0; str[i] != '\0'; i++) tempBucket._len++;

    tempBucket._str = strcat(tempBucket._str, str);

    tempBucket._str[len] = '\0';
    tempBucket._len = len;
    return tempBucket;
}

const Bucket& Bucket::operator=(const char* str) {
    if (this->_str == str) {
        return *this;
    }
    
    _len = 0;
    for (int i = 0; str[i] != '\0'; i++) _len++;

    _str = new char[_len + 1];
    for (int i = 0; i < _len; i++)
        _str[i] = str[i];
    _str[_len] = '\0';

    return *this;
}

const bool Bucket::operator==(const char* str) {
    int comp = strcmp(this->_str, str);
    if (comp == 0) {
        return true;
    }
    else {
        return false;
    }
}

ostream& operator<<(ostream& os, const Bucket& myBucket) {
    os << myBucket._str;
    return os;
}

istream& operator>>(istream& is, const Bucket& myBucket) {
    static char buffer[40];
    is >> buffer;

    int len = 0;
    for (size_t i = 0; buffer[i] != '\0'; i++) {
        myBucket._str[i] = buffer[i];
        len++;
    }
    myBucket._str[len++] = '\0';

    return is;
}

Main:

int main()
{
    Bucket b1("Hello, "); // This is deleted by the destructor
    Bucket b2(b1); // This is deleted by the destructor
    cout << b1 << b2 << endl; 
    b2 = "Dear ";  // Also works fine
    Bucket b3;
    cout << "Enter a name: ";
    cin >> b3; // Can't assign the _len attribute
    cout << b2 + b3 << ",";  // not destroyed
    Bucket b4(" Please write this sentence after pressing enter:\n");
    b2 = "We believe that ";
    cout << b4 + b2 << b1 << "and " << "Goodbye "  // not destroyed
        << (b1 == "Goodbye " ? "is " : "is not ") << "the same word!\n" <<
        endl;
    return 0;
}

This question's shown code fails to meet Stackoverflow's requirements for showing a [mre]. Because of that it's unlikely that anyone here can conclusively answer the question; but only guess at the most. You need to [edit] your question to show a minimal example, no more than one or two pages of code (the "minimal" part), that everyone else can cut/paste ***exactly as shown***, compile, run, and reproduce the described issue (the "reproducible" part, this includes any ancillary information, like any input to the program). See [ask] for more information. — Sam Varshavchik, Apr 28 '22 at 00:57
Be sure to also read Sam's advice. But at a glance, I see a destructor and no copy operators, so you should also read the C++-specific page [What is the Rule of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — Silvio Mayolo, Apr 28 '22 at 01:00
Fyi, `if (_len > 0) delete[] _str;` that precondition was not present in the default ctor when you allocated a sequence of zero-length and stashed the resulting pointer; it should not conditioned in the destructor either. I concur with the Ro3 references, btw. Important. — WhozCraig, Apr 28 '22 at 01:16
Also , don't call new[] with zero size to initialize pointer .Yes, it's defined and result is a non-null pointer , dereferencing it invokes UB though but guards against it do not exist. It also must be deleted. — Swift - Friday Pie, Apr 28 '22 at 01:18

Remy Lebeau · Accepted Answer · 2022-04-28T04:07:25.403

I see a lot of issues with how your Bucket code is implemented:

the 2nd parameter of operator>> needs to be a reference to a non-const object, otherwise the operator can't modify the object while reading data from the istream.
lack of a copy constructor and copy assignment operator, and preferably also a move constructor and move assignment operator, per the Rule of 3/5/0.
the default constructor is allocating memory with new[], but since _len is 0, the destructor won't free that memory. You need to remove the check for _len > 0 when calling delete[].
the default constructor is not null-terminating the allocated array, as the other constructors are doing.
operator+ is returning a new Bucket object by value (as it should be), so marking that object as const is redundant.
Same with the bool that operator== returns. However, the operator itself should be marked as const, since it doesn't modify the Bucket object it is called on.
both operator+ are using strcat() incorrectly. The Bucket overload is modifying the contents of this->str_, which it should not be doing, and worse this->_str hasn't been reallocated anyway to increase its capacity to append the contents of myBucket._str. The char* overload is making a similar mistake with tempBucket.str_. You need to allocate a whole new char[] array for each tempBucket.
the Bucket overload of operator+ is declaring tempBucket as String instead of as Bucket.
operator= is returning the modified Bucket object by reference (as it should be), but that object should not be maarked as const.
Bucket does not expose a way to access its _str member from outside code, so there is no way that the if (this->_str == str) check in operator== will ever be true.
operator= is not delete[]'ing the old _str memory before replacing it with a new char[].
operator>> is not performing any bounds checking on the buffer it reads in. And it is not re-allocating myBucket._str to fit the contents of buffer. And, it is counting the null terminator in _len, which none of the other class methods do.

With all of that said, try something more like this instead:

class Bucket
{
    friend ostream& operator<<(ostream& os, const Bucket& rhs);
    friend istream& operator>>(istream& is, Bucket& rhs);
public:
    Bucket(size_t len = 0);
    Bucket(const Bucket& src);
    Bucket(Bucket&& src);
    Bucket(const char* str);
    Bucket(const char* str, size_t len);
    ~Bucket();

    Bucket operator+(const Bucket& rhs) const;
    Bucket operator+(const char* str) const;

    /*
    Bucket& operator=(const Bucket& rhs);
    Bucket& operator=(Bucket&& rhs);
    Bucket& operator=(const char* str);
    */
    Bucket& operator=(Bucket rhs);

    bool operator==(const Bucket& rhs) const;
    bool operator==(const char* rhs) const;

private:
    char* _str;
    size_t _len;

    void swap(Bucket &other);
    bool equals(const char* str, size_t len) const;
    Bucket concat(const char* str, size_t len) const;
};

static size_t my_strlen(const char* str) {
    const char* start = str;
    if (str) while (*str != '\0') ++str;
    return (str - start);
}

Bucket::Bucket(size_t len) {
    _len = len;
    if (len > 0) {
        _str = new char[len + 1];
        _str[len] = '\0';
    }
    else {
        _str = nullptr;
    }
}

Bucket::Bucket(const Bucket& src)
    : Bucket(src._str, src._len) { }

Bucket::Bucket(Bucket&& src) : Bucket(0) {
    src.swap(*this);
}

Bucket::Bucket(const char* str)
    : Bucket(str, my_strlen(str)) { }
    
Bucket::Bucket(const char* str, size_t len) : Bucket(len) {
    if (str && len > 0) {
        for(size_t i = 0; i < len; ++i) {
            _str[i] = str[i];
        }
    }
}

Bucket::~Bucket() {
    delete[] _str;
}

void Bucket::swap(Bucket &other) {
    char *ptmp = _str;
    _str = other._str;
    other._str = ptmp;

    size_t itmp = _len;
    _len = other._len;
    other._len = itmp;
}

bool Bucket::equals(const char* str, size_t len) const {
    if (this->_len != len) return false;
    for(size_t i = 0; i < len; ++i) {
        if (this->_str[i] != str[i]) return false;
    }
    return true;
}

Bucket Bucket::concat(const char* str, size_t len) const {
    Bucket tempBucket(this->_len + len);
    for(size_t i = 0; i < this->_len; ++i) {
        tempBucket._str[i] = this->_str[i];
    }
    for(size_t i = this->_len, j = 0; j < len; ++i, ++j) {
        tempBucket._str[i] = str[j];
    }
    return tempBucket;
}

Bucket Bucket::operator+(const Bucket& rhs) const {
    return concat(rhs._str, rhs._len);
}

Bucket Bucket::operator+(const char* rhs) const {
    return concat(rhs, my_strlen(rhs));
}

/*
Bucket& Bucket::operator=(const Bucket& rhs) {
    if (this != &rhs) {
        Bucket(rhs).swap(*this);
    }
    return *this;
}

Bucket& Bucket::operator=(Bucket&& rhs) {
    Bucket(std::move(rhs)).swap(*this);
    return *this;
}

Bucket& Bucket::operator=(const char* rhs) {
    Bucket(rhs).swap(*this);
    return *this;
}
*/

Bucket& Bucket::operator=(Bucket rhs) {
    rhs.swap(*this);
    return *this;
}

bool Bucket::operator==(const Bucket& rhs) const {
    return equals(rhs._str, rhs._len);
}

bool Bucket::operator==(const char* rhs) const {
    return equals(rhs._str, my_strlen(rhs));
}

ostream& operator<<(ostream& os, const Bucket& rhs) {
    os.write(rhs._str, rhs._len);
    return os;
}

istream& operator>>(istream& is, Bucket& rhs) {
    /*
    string buffer;
    is >> buffer;
    rhs = buffer.c_str();
    return is;
    */
    char buffer[40];
    if (!is.get(buffer, 40)) buffer[0] = '\0';
    rhs = buffer;
    return is;
}

That being said, if you can use standard C++ functionalities, such as std::string, then the code becomes a whole lot simpler:

#include <string>

class Bucket
{
    friend ostream& operator<<(ostream& os, const Bucket& rhs);
    friend istream& operator>>(istream& is, Bucket& rhs);
public:
    Bucket() = default;
    Bucket(const Bucket& src) = default;
    Bucket(Bucket&& src) = default;
    ~Bucket() = default;

    Bucket(const std::string& str);

    Bucket operator+(const Bucket& rhs) const;

    Bucket& operator=(const Bucket& rhs) = default;
    Bucket& operator=(Bucket&& rhs) = default;

    bool operator==(const Bucket& rhs) const;

private:
    std::string _str;
};

Bucket::Bucket(const std::string str) : _str(str) { }

Bucket Bucket::operator+(const Bucket& rhs) const {
    return Bucket(this->_str + rhs._str);
}

bool Bucket::operator==(const Bucket& rhs) const {
    return (this->_str == rhs._str);
}

ostream& operator<<(ostream& os, const Bucket& rhs) {
    os.write << rhs._str;
    return os;
}

istream& operator>>(istream& is, Bucket& rhs) {
    is >> rhs._str;
    return is;
}

How to use delete[] in destructor

1 Answers1