Cppcoreguidelines: It is not clear why the example in C.121 leads to a resource leak

[bostikforever]

In the derived class, string s in statically allocated, so that it is properly cleaned up in the event of a delete.
I think the example should read instead:

#include <memory>
#include <string>

using namespace std;

class Goof {
public:
    // ...only pure virtual functions here ...
    // no virtual destructor
};

class Derived : public Goof {
    string* s;
public:
    Derived(string s):s{new string{s}}
    {
    }
    ~Derived()
    {
        delete s;
    }
    // ...
};

void f(Goof* p) {
}

void g(Goof* p) {
}

void use()
{
    unique_ptr<Goof> p {new Derived{"here we go"}};
    f(p.get()); // use Derived through the Goof interface
    g(p.get()); // use Derived through the Goof interface
} // leak

int main() {
    use();
    return 0;
}

[djarek]

Note that the unique_ptr p deletes through a pointer to a Goof (base class) that does not have a virtual destructor, which results in the destructors of derived class (~Derived() in this case) not being executed, so the result is a leak.

[bostikforever]

In the original example, the string s is not dynamically allocated, so there cannot be a leak even though ~Derived() will not be called.

[djarek]

Not running ~Derived() will result in std::string::~string() not being run. Properly cleaning up a std::string's memory requires calling its destructor (I'm not considering Small String Optimization here).
So any memory allocated by the string internally will leak in the example.

[bostikforever]

I agree with what you have said but the point that the example does not necessarily lead to a leak still stands. I think that this fact is distracting from the main point.

[KingAkeem]

Wouldn't switching to make_unique<Derived>("here we go") solve the issue since make_unique will automatically run~Derived()?

[JonasToth]

Wouldn't switching to make_unique("here we go") solve the issue since make_unique will automatically run~Derived()?

Yes, but the point is that we want polymorphic instances of Goof and substitute the best matching class depending on some condition. We only know the interface necessary but not the implementation.

Therefore we need unique_ptr<Goof> to make sure the interface is correct but initialize it with one of its subclasses.

[KingAkeem]

I understand that you're using Goof as an interface but what I'm saying is that instead of allocating the Derived object using new, why can't you use unique_ptr<Goof> p = make_unique<Derived>("here we go")? I'm fairly new to C++ so forgive my ignorance.

[JonasToth]

why can't you use unique_ptr p = make_unique("here we go")?

Using make_unique has the same behaviour. And thats not ignorance, C++ has its pitfalls with these issues which make it sometimes hard to understand.

The only way to fix the issue is adding virtual ~Goof() = default; to the class Goof. If you are using g++ or clang++ you can test the difference copy and pasting the code from above.

If you compile like this:
g++ test_make_unique.cpp -o test_make_unique.x --std=c++14 -fsanitize=address and execute the program with ./test_make_unique.x the sanitizer will give you the leak error without the virtual destructor.

For reference:

→ ./test_make_unique.x 
=================================================================
==3875==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x60200000eff0 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   8 bytes;
  size of the deallocated type: 1 bytes.
    #0 0x7fba046a8132 in operator delete(void*, unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9a132)
    #1 0x4019a6 in std::default_delete<Goof>::operator()(Goof*) const (/home/jonas/Programme/test/test_make_unique.x+0x4019a6)
    #2 0x401573 in std::unique_ptr<Goof, std::default_delete<Goof> >::~unique_ptr() (/home/jonas/Programme/test/test_make_unique.x+0x401573)
    #3 0x400e2f in use() (/home/jonas/Programme/test/test_make_unique.x+0x400e2f)
    #4 0x400e9e in main (/home/jonas/Programme/test/test_make_unique.x+0x400e9e)
    #5 0x7fba03ccc82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #6 0x400c58 in _start (/home/jonas/Programme/test/test_make_unique.x+0x400c58)

0x60200000eff0 is located 0 bytes inside of 8-byte region [0x60200000eff0,0x60200000eff8)
allocated by thread T0 here:
    #0 0x7fba046a7532 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x99532)
    #1 0x4010b9 in _ZSt11make_uniqueI7DerivedIRA11_KcEENSt9_MakeUniqIT_E15__single_objectEDpOT0_ (/home/jonas/Programme/test/test_make_unique.x+0x4010b9)
    #2 0x400dd6 in use() (/home/jonas/Programme/test/test_make_unique.x+0x400dd6)
    #3 0x400e9e in main (/home/jonas/Programme/test/test_make_unique.x+0x400e9e)
    #4 0x7fba03ccc82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: new-delete-type-mismatch ??:0 operator delete(void*, unsigned long)
==3875==HINT: if you don't care about these warnings you may set ASAN_OPTIONS=new_delete_type_mismatch=0
==3875==ABORTING

The sanitizer shows that the wrong destructor is called for the code. Thats because you initialize with Derived but delete it as Goof.

The error message here is with unique_ptr<Goof> p = std::make_unique<Derived>("here we go"); in the code. But both variants result in the same issue.

The problem is that unique_ptr can only call the destructor of its template type (Goof in our example). If that destructor is not virtual it can not dynamically dispatch to the destructor of the child class (Derived here). I am not sure if I explained it good enough but try to experiment a little with it :)

[KingAkeem]

I think I understand, what you're saying is only ~Derived() will run but since Goof doesn't have a destructor then unique_ptr<Goof> p will never be deleted. I'm going to test it out now and see if I can figure it out.

[JonasToth]

Here is an example:

#include <iostream>
#include <memory>
#include <string>

using namespace std;

class Goof {
public:
    // ...only pure virtual functions here ...
    // Make this Destructor virtual/non-virtual to observe different behaviour in destruction
    virtual ~Goof() { std::cout << "Base class destructor" << std::endl; }
};

class Derived : public Goof {
    string* s;
public:
    Derived(string s):s{new string{s}}
    {
    }
    ~Derived() {
        delete s;
        std::cout << "Child class destructor" << std::endl;
    }
    // ...
};

void f(Goof* p) {
}

void g(Goof* p) {
}

void use()
{
    // unique_ptr<Goof> p {new Derived{"here we go"}};
    unique_ptr<Goof> p  = std::make_unique<Derived>("here we go");
    f(p.get()); // use Derived through the Goof interface
    g(p.get()); // use Derived through the Goof interface
} // leak

int main() {
    use();
    return 0;
}

[KingAkeem]

Thank you so much! I never quite understand virtual functions but you've helped me a lot with this example. I think I'm going to play around with it some more but I understand it more now. If I add a ~Goof() then instead of dynamically dispatching to ~Derived(), ~Goof() will be called. But if I create a virtual ~Goof() that tells the compiler to look at the object type that the unique_ptr <Goof> p points to and use that destructor, which is ~Destructor(). Is that about right?

[JonasToth]

If I add a ~Goof() then instead of dynamically dispatching to ~Derived(), ~Goof() will be called.

Yes. ~Goof() is added by the compiler automatically if not specified differently. Thats why the default is non-virtual and therefore the bug source is sutle.

But if I create a virtual ~Goof() that tells the compiler to look at the object type that the unique_ptr p points to and use that destructor, which is ~Destructor(). Is that about right?

It is about right. What is at work is not 'the compiler looking', but the dynamic polymorphism mechanism that happens with normal virtual methods, too.

virtual calls are not direct calls like normal functions, but indirect ones. This is implemented with the vtable.

I think this Article gives a nice introduction. This image shows whats going on (from the same article).

For virtual methods the compiler does not know which function to call, but it knows the spot of the function address within the vtable. So instead of calling a function the program will consult the content of the vtable at the spot for the wanted function and call the address living there.

[KingAkeem]

About your first comment, I'm a little confused. What do you mean by ~Derived() will call the base class destructor, too? I thought the Base class destructor was being called because of unique_ptr<Goof> p? Also thanks for the article, I'm about to read it now and hopefully it'll clear things up for me.

[JonasToth]

If you have a inheritence hierarchy every class can introduce new data, constraints or similar stuff that must be reversed by its destructor. Therefore a destructor will not only do its own stuff but also call its baseclass destructor. Otherwise you would only delete the last 'slice' of a object and not everything.

The problem with unique_ptr<BaseClass> is that only the first slice is deleted and all slices added by child classes NOT. Doing virtual BaseClass() starts at the bottom and goes up until everything of the object is deleted.

[KingAkeem]

Yes I agree with that, I think I just misunderstood what you were saying. I read the article also, it's cleared up a lot, thank you!

[Wolf-at-SO]

@JonasToth starts at the bottom and goes up <-- that's a bit unclear. I'd say, destruction starts with the destructor of actual class (stored in the vtable) and then it goes to the base class and their base class(es).

[JonasToth]

@JonasToth starts at the bottom and goes up <-- that's a bit unclear. I'd say, destruction starts with the destructor of actual class (stored in the vtable) and then it goes to the base class and their base class(es).

Yes. Maybe in reverse order of construction would be a good formulation too.

In general: Is there a good explanation of virtual calls in the document already? Maybe it would clarify such issues.

[MikeGitb]

I think the question of leakage is deceptive: Deleting an object (that hasn't a virtual destructor) via a pointer to a base class is undefined behavior - period. It doesn't matter, if the derived class adds any datamembers that could leak anything or even if the destructor of the derived class does any additional work at all:

5.3.5 Delete [expr.delete]
[...]
3) In the first alternative (delete object), if the static type of the object to be deleted is different from its
dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the static type shall have a virtual destructor or the behavior is undefined. [...]

[jwakely]

IMHO the example is "correct" in that it does show the problem described (it has undefined behaviour as MikeGitb says, which in practice will fail to destroy the std::string member, leaking any memory it allocated). This is the problem explained in C.35.

So the OP's point is wrong, there is a leak in the example, and the text following it is entirely accurate:

The Derived is deleted through its Goof interface, so its string is leaked. Give Goof a virtual destructor and all is well.

However, I'm not really sure that the example is a good one for "If a base class is used as an interface, make it a pure abstract class". It's a good example for C.35 or C.127 but they both have similar examples already.

So I think either C.121 should have an example more relevant to that rule, or should at least link to C.35 for further explanation of the undefined behaviour.

[bostikforever]

@jwakely The undefined behavior for non-virtual base class destructors is not necessarily a leak, so if a leak is being highlighted this should be clear from the example. In this example in particular, there is not necessarily a heap allocation going on. I think the example can be improved by making the leak obvious e.g using a heap allocated resource as I have shown, increasing the string length so that it is internally heap allocated, or annotating the example with your reasoning ("even though in this case there is possibly no heap allocation going on, there is in general a possibility that data members would have heap allocation and/or other resources")

[jwakely]

In the general case std::string performs heap allocations. How do you know the example doesn't allocate for the string? Maybe the implementation doesn't support the Small String Optimization, or "here we go" is longer than its small string buffer, or the class stores "here we go" + " some more words that make the string much longer".

And the text doesn't say "memory leak", it says "leak", which could be referring to more general resource leaks, including failing to run a class destructor. The string is not destroyed, so it is "leaked". If it owns dynamically allocated memory then that memory will be leaked, but "leak" doesn't always mean "memory leak".

("even though in this case there is possibly no heap allocation going on, there is in general a possibility that data members would have heap allocation and/or other resources")

That would not be an improvement.

The example and the text are correct as written (but I still think they are not very applicable to C.121, but they are technically correct).

[bostikforever]

which could be referring to more general resource leaks

It either is, or isn't. I do not see how imprecision leads to technical correctness.

I am not familiar with this general definition of 'leak', being a failure to run a class destructor.

I guess the ambiguity for me is in distinguishing between a possible leak and a definite one. It seems here that the leak in this example refers to a possible leak (if we do not include your arbitrary definition of a leak i.e. failure to run a destructor), whereas I was expecting a definite leak. Alas, the string does not even have to be an std::string.

If you believe that there is no improvement to be made feel free to close this issue.