Problems with shared_ptr and operator new

Question

I get a bug in a test code, where I have to test a class located at the bottom of a diamond heritage, and above all, which forms a circular dependency with an other class, too difficult to change (I promise it is not my code...).

The constructor of a class A ask for a reference to a class B, and the class B asks for a shared_ptr to the class A...
For unit tests purpose, I'd like to break this dependency, by allocating the memory for A, but without building the instance. Then by passing this shared_ptr to B, and once B is built, by building the A instance whose constructor would take the reference to B.

I succeded to reproduce the problem in a sample code you can execute locally or any online compiler :

#include <iostream>
#include <memory>
using namespace std;

struct ILevel_0
{
    virtual ~ILevel_0() = 0;
};
ILevel_0::~ILevel_0() {}

struct Level_1_A : virtual ILevel_0
{
    int a;
};

struct Level_1_B : virtual ILevel_0
{
    int b;
};

struct Level_2 : Level_1_A, Level_1_B
{
    int c;
};

struct OtherStruct
{
    OtherStruct(const std::shared_ptr<ILevel_0>& lev) : _lev(lev) {}
    std::shared_ptr<ILevel_0> _lev;
};

int main()
{
    std::shared_ptr<Level_2> level2;
    
    void* level2_rawMemory = operator new(sizeof(Level_2));
    Level_2* level2Ptr = static_cast<Level_2*>(level2_rawMemory);
    level2.reset(level2Ptr);
    
    std::cout << "------ 1" << std::endl;
    
    std::shared_ptr<ILevel_0> level0 = level2;  // OK
    
    std::cout << "------ 2" << std::endl;

    // OtherStruct otherStruct(level2);   // KO  : crash after ------ 1
    
    std::cout << "------ 3" << std::endl;

    // Needed, else the shared_ptr's deleter would crash by calling the delete instruction
    new (level2.get()) Level_2{};

    std::cout << "------ 4" << std::endl;
    
    return 0;
}

You can see a line (building an OtherStruct instance) is commented out, so that the code code can be compiled and run wihout crashing. If you uncomment it, the program will crash, without printing "------ 2". I don't understand why, and I don't understand why the previous instruction (level0 building) doesn't crash.

Thanks for help, and sorry for approximative english.

The code segvs for me even _without_ decommenting that line. This means you're already in undefined behavior world, so it's a bit pointless, imho, to search what's wrong with the commented line, because there's something wrong with all the rest. — Enlico, Dec 12 '20 at 23:30
You can flush `std::cout` to print all output before moving to the next line — fdermishin, Dec 12 '20 at 23:32
@fdermishin, I think [`std::endl` is already flushing](https://stackoverflow.com/a/213977/5825294). — Enlico, Dec 12 '20 at 23:33
When a program contains **undefined behavior**, it is pointless to reason about why you get unexpected results. Also, there are almost no valid reason to call `operator new` directly. — Phil1970, Dec 13 '20 at 00:09

sehe · Answer 1 · 2020-12-12T23:49:31.353

4

Run your code under ASAN/UBSAN:

Stepping through reveals that

39          std::shared_ptr<ILevel_0> level0 = level2; // OK

Is the culprit.

However, the real offender is this:

    void* level2_rawMemory = operator new(sizeof(Level_2));
    auto* level2Ptr = static_cast<Level_2*>(level2_rawMemory);
    level2.reset(level2Ptr);

It violates the C++ memory model. You can't just reinterpret raw memory as a Level_2 object. Especially when the data types are not POD (they're not, since they're virtual).

Instead use

    Level_2* level2Ptr = new Level_2;
    level2.reset(level2Ptr);

Or indeed

    level2.reset(new Level_2);

Best of all:

std::shared_ptr<Level_2> level2 = std::make_shared<Level_2>();

Actually consider safe pointer casts (see Using boost::function with a parameter to shared pointer to derived class):

std::shared_ptr<ILevel_0> level0 = dynamic_pointer_cast<ILevel_0>(level2); // OK

DEMO With Fixes

Live On Coliru

#include <iostream>
#include <memory>
#include <utility>

struct ILevel_0 { virtual ~ILevel_0() = default; };

struct Level_1_A : virtual ILevel_0 { int a{}; };
struct Level_1_B : virtual ILevel_0 { int b{}; };

struct Level_2 : Level_1_A, Level_1_B { int c{}; };

struct OtherStruct {
    explicit OtherStruct(std::shared_ptr<ILevel_0> lev) : _lev(std::move(lev)) {}
    std::shared_ptr<ILevel_0> _lev;
};

int main() {
    std::cout << std::unitbuf;
    std::shared_ptr<Level_2> level2 = std::make_shared<Level_2>();

    std::cout << "------ 1" << std::endl;
    auto level0 = std::dynamic_pointer_cast<ILevel_0>(level2); // OK

    std::cout << "------ 2" << std::endl;

    OtherStruct otherStruct(level2);

    std::cout << "------ 3" << std::endl;

    level2 = std::make_shared<Level_2>();

    std::cout << "------ 4" << std::endl;
}

Prints

------ 1
------ 2
------ 3
------ 4

And no sanitizer warnings.

edited Dec 12 '20 at 23:49

answered Dec 12 '20 at 23:36

sehe

374,641
47
450
633

Added a demo with fixes **[Live On Coliru](http://coliru.stacked-crooked.com/a/58a3744d98aa9bda)** – sehe Dec 12 '20 at 23:43
First of all, thank for the answer and the spent time. You suggest to do : ```Level_2* level2Ptr = new Level_2;``` But it does instantiate a Level2 object, and as said in my introduction, in my real code, I can't do that, because of circular dependency. That's why I want just to allocate the memory, pass the pointer, and instantiate afterwards. That's the whole point of my problem, and that's why I use `operator new`. – Oodini Dec 13 '20 at 17:04
By the way, if you know documentation about C++ memory model, I'd like to read it. – Oodini Dec 13 '20 at 17:13
Best documentation is in the standard. However, I'd be temtped to guess you come from a C background and accidentally assumed C++ is "pretty much C with classes". That's a fallacy. I'd suggest a book like "Accelerated C++" (Moo/Koenig). Alternatively, cppreference.com is a very good hyperlinked reference on the language: https://en.cppreference.com/w/cpp/language/object – sehe Dec 13 '20 at 19:25
"I can't do that, because of circular dependency" - I can't imagine the problem. You know that you can use `this` inside the constructor, right (some limitations apply w.r.t. the order of initialization of subobjects/derived). Otherwise just use a 2-step initialization function (it's an antipattern, but since you didn't describe why you need it, perhaps you do, giving you the benefit of the doubt) – sehe Dec 13 '20 at 19:29
You can't imagine how the code is messy and complicaetd to modify. The whole code is an antipattern. I just gave you a diamond, but there are 8 levels in the hierarchy, and I am not allowed by the architect to modify anything, because he fears that all these gems explode at his face. But I try anyway to do some unit tests. So I rephrase the question : is it possible to use placement new with class at the bottom of a diamond (so with virtual inheritance), and espacially when using shared_ptr ? – Oodini Dec 13 '20 at 20:42
Yes. Placement new exists. Beware of alignment requirements. Specifically, don't do what you did: don't use a pointer to uninitialized memory as if it is initialized. See also [`std::launder`](https://en.cppreference.com/w/cpp/utility/launder). However, if you need to know these nooks and crannies (I donot) I think you're probably swimming against the stream. – sehe Dec 13 '20 at 20:50
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/225908/discussion-between-sehe-and-oodini). – sehe Dec 13 '20 at 20:50

Problems with shared_ptr and operator new

1 Answers1

DEMO With Fixes