Why does this explicit destructor cause memory corruption in a shared ptr?

Question

What is wrong with this code and how can I fix it?

#include <iostream>
#include <boost/shared_ptr.hpp>
#include <vector>

struct CTest
{
    CTest()
    { std::cout << "ctor CTest" <<std::endl; }

    ~CTest()
    { std::cout << "dtor CTest" <<std::endl; }
};

struct CSlot
{
    CSlot() : m_test(new CTest()), m_num(123)
    { }

    ~CSlot()
    {
        // m_test.reset(); // this line fixed the code but I don't know why
        m_num = -1;
    }

    boost::shared_ptr<CTest> m_test;
    int m_num;
};

int main()
{
    std::vector<CSlot> testVector(1);

    std::cout << "1" << std::endl;
    new (&testVector[0]) CSlot();

    // clear slot
    testVector[0].~CSlot();
    std::cout << "2" << std::endl;
}

this code looks like working, and prints:

ctor CTest
1
ctor CTest
dtor CTest
2

but sometimes the program crash and valgrind always says:

==13372== Invalid read of size 4
==13372==    at 0x400D8F: boost::detail::atomic_exchange_and_add(int*, int)
...

I can fix this behavior with the m_test.reset() line, but I think there is a more correct solution...

score 6 · Accepted Answer · answered Dec 29 '11 at 10:12

6

Because what you're doing makes no sense. You're creating an object, and then... creating an object on the same address.

Then you're destroying the object on that address... And then you're destroying it again.

How is that supposed to work?

You asked for a vector of CSlot objects, so that's what you got. You asked for it to have size 1, so it contains one CSlot object, fully constructed and ready for action. So it doesn't make sense to construct a CSlot object on top of it.

If you want to use placement new and call the destructor directly, you should do it into an empty char buffer.

answered Dec 29 '11 at 10:12

jalf

243,077
51
345
550

I want to create some slot with default values, and later initialize with placement new and sometimes reset with explicit destructor... Maybe I need an other constructor for the shared_ptr – Industrial-antidepressant Dec 29 '11 at 10:17
"If you want to use placement new and call the destructor directly, you should do it into an empty char buffer." Not exactly. You have to use properly aligned raw memory. But char buffer is OK if you make sure it is properly aligned. – Serge Dundich Dec 29 '11 at 10:18
@SergeDundich: I was trying to keep it simple, and a char[] is the most common way to do this. ;) – jalf Dec 29 '11 at 10:19
2

@Industrial-antidepressant: that's evil. You'll generally just want to have *one* object which can be modified at will. Destroying the one creating by the vector, and then creating your own is just nasty. If you want to reset it, add a reset member function to the class, or perform an assignment like this: `testVector[0] = CSlot()` – jalf Dec 29 '11 at 10:20
@jalf: No, I dont want to create a temporary, the reset method is the solution – Industrial-antidepressant Dec 29 '11 at 10:26
@Industrial-antidepressant: "later initialize with placement new and sometimes reset with explicit destructor" What makes you think that you have to use placement new and explicit destructor? You should never use placement new at the position of already constructed object. You should never call explicit destructor for a scoped object or object owned by some other class (like `vector` here). There are lots of good C++ features like assignment operators, member functions,... that will do the job correctly. Just don't use placement new and explicit destructor if you don't know what you are doing. – Serge Dundich Dec 29 '11 at 10:28
@Serge Dundich: I refactored this code... The original vector contained shaed_ptr and I started to rewrite it mechanically.. So where the original code was testVector[0].reset() I started to replace with explicit destructor and so on.. – Industrial-antidepressant Dec 29 '11 at 10:34

Why does this explicit destructor cause memory corruption in a shared ptr?

1 Answers1