How to safely free memory when working with inheritance and polymorphism?

Question

I don't know how to correctly free obj2 in this code sample below, I tried using virtual destructor but exception was raised regarding the char pointer m_s not being allocated.

#include <iostream>
using namespace std;
class A 
{
private:
    char *m_s;
public:
    A() { m_s = strdup("default"); }
    A(char *s) { del_data(); m_s = strdup(s); }
    A(const A& a) { del_data(); m_s = strdup(a.m_s); }
    virtual void prepare() { cout << "A "; }
    void display() {
    prepare();
    cout << m_s << endl;
    }

    void del_data()
    {
        if (m_s)
        {
            free(m_s);
        }

        m_s = nullptr;
    }

    virtual ~A()
    {
        del_data();
    }
};

class B : public A 
{
public:
    B(char *s) : A(s) { }
    B(const B &b) : A(b){}
    void prepare() { cout << "B "; }

    ~B() {
        A::del_data();
    }
};

void foo(A *obj1, A obj2) {
    obj1->display();
    obj2.display();
}


int main() {
    B obj1("text");
    A *obj2 = new B(obj1);
    foo(&obj1, *obj2);

    delete obj2;

    return 0;
}

When I delete obj2, the destructor of class B should be called and and del_data() method should free memory allocated and set m_s to nullptr. Somehow it does not work as intended, I've even tried using delete[] to no avail.

I would love to know what I'm doing wrong and how to avoid this mistake in the future.

Thank you for your time.

`A(char *s) { del_data(); m_s = strdup(s); }` -> This deletes an initialised pointer. Same with your other constructor. You also need to define a copy-constructor. — Mike Vine, Jun 22 '23 at 07:41
Use std::string for your member, and std::unique_ptr and std::make_uniqe. Note that in current C++ code new/delete should only be necessary inside datastructures. Otherwise use std::unique_ptr or standard library containers. — Pepijn Kramer, Jun 22 '23 at 07:43
Is it because the initial value of m_s is not nullptr by default. As a result, when I try to call the constructor A(char* s), m_s is not null so I was deleting unallocated memory right? — Juno Lee, Jun 22 '23 at 07:53
[What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) — 463035818_is_not_an_ai, Jun 22 '23 at 07:58
you should always initialize all members as the very first thing in constructors. You seem to assume that `m_s` is automagically initialized with `nullptr` but it is not — 463035818_is_not_an_ai, Jun 22 '23 at 08:01
Your `A` class is missing a user-defined assignment operator. If you used `std::string`, you wouldn't need a user-defined copy constructor, assignment operator, and the virtual destructor could simply be `virtual ~A() = default;` — PaulMcKenzie, Jun 22 '23 at 08:07
Do not use explicit `new`! We have 2023 so you should use smart pointers (C++11) and `std::make_unique` (since C++14) `std:make_shared` (since C++11). And use `std::string` to handling strings - this is very old feature ('94 before formal standard was defined). — Marek R, Jun 22 '23 at 08:52
Also you have "object slicing" since you are passing second argument by copy. — Marek R, Jun 22 '23 at 09:02

score 0 · Answer 1 · answered Jun 22 '23 at 08:06

Many issues in your code but the segfault comes from m_s not being initialized thus you called del_data on a random address. Other issues with constness and properly overriding virtual function. You don't need to implement ~B() like you did: ~A() will be automatically called.

Yet, in terms of design, you are using C-like string API. I strongly advise moving to std::string that will save you a lot of trouble in this situation.
Below the fix with C strings. I leave you the conversion to std::string as an exercise ;)

#include <iostream>
#include <string.h>

using namespace std;
class A 
{
private:
    char *m_s = nullptr;
public:
    A() { m_s = strdup("default"); }
    A(char const * const s) { del_data(); m_s = strdup(s); }
    A(const A& a) { del_data(); m_s = strdup(a.m_s); }
    virtual void prepare() const { cout << "A "; }
    void display() {
    prepare();
    cout << m_s << endl;
    }

    void del_data()
    {
        if (m_s)
        {
            free(m_s);
        }

        m_s = nullptr;
    }

    virtual ~A()
    {
        del_data();
    }
};

class B : public A 
{
public:
    B(char const * const s) : A(s) { }
    B(const B &b) : A(b){}
    // override virtual prepare()
    void prepare() const override { cout << "B "; }

    ~B() =default;
};

void foo(A *obj1, A obj2) {
    obj1->display();
    obj2.display();
}


int main() {
    B obj1("text");
    A *obj2 = new B(obj1);
    foo(&obj1, *obj2);

    delete obj2;

    return 0;
}

Live

score 0 · Answer 2 · answered Jun 22 '23 at 08:12

if in del_data() called in constructor is called with uninitialized m_s (its value is unspecified) Initialization to nullptr will fix it.

class A 
{
private:
    char *m_s = nullptr;
...

But there is no reason to call it in constructor - it will never point to memory that has to be freed.

How to safely free memory when working with inheritance and polymorphism?

2 Answers2