std::make_shared leads to undefined behavior, but new works

Question

Consider the following example class:

class Foo {

public:
    void* const arr_;

    Foo() = delete;

    Foo(const size_t size, bool high_precision) :
        arr_(Initialize(size, high_precision)) {};

    template <typename T>
    T* GetDataPointer() {
        return (T* const)arr_;
    }
private:
    static void* Initialize(const size_t size, bool high_prec) {
        if (high_prec) {
            return new double[size];
        }
        else {
            return new float[size];
        }
    }

};

When I created a shared pointer to a Foo object using std::make_shared, I often find the array data I initialize displays undefined behavior/becomes corrupted. For example:

std::shared_ptr<Foo> sp_foo = std::make_shared<Foo>(Foo(3,false));
    auto foo_data = sp_foo->GetDataPointer<float>();
    foo_data[0] = 1.1;
    foo_data[1] = 2.2;
    foo_data[2] = 3.3;
    std::cout << "First Value: " << 
        sp_foo->GetDataPointer<float>()[0]; // Sometimes this is 1.1, sometimes it is meaningless.

However, when I initialize the shared pointer using new, this problem seems to go away.

std::shared_ptr<Foo> sp_foo (new Foo(3,false));
    auto foo_data = sp_foo->GetDataPointer<float>();
    foo_data[0] = 1.1;
    foo_data[1] = 2.2;
    foo_data[2] = 3.3;
    std::cout << "First Value: " << 
        sp_foo->GetDataPointer<float>()[0]; // Correctly prints 1.1

Any thoughts as to what is happening? Small side note: I am constrained to not template the Foo class and to indeed have a void* const to the onboard data array.

Looks like you are using copy constructor with `make_shared`, which is autogenerated by the compiler, hence wrong, because your class is using `new[]`; Try `std::make_shared(3, false);` instead — pptaszni, Apr 05 '22 at 13:03
You should probably also fix or delete the copy constructor. — super, Apr 05 '22 at 13:14
To be explicit - your class `Foo` doesn't consider [The Rule of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) - it needs a destructor, and therefore needs other member functions as well. And `std::make_shared` already calls a constructor, so `std::make_shared(Foo(3,false))` unnecessarily calls _two_ Foo constructors. — Drew Dormann, Apr 05 '22 at 13:15
Re: "displays undefined behavior" -- undefined behavior isn't a thing that happens; it's a description of your program. Under some circumstances the language definition doesn't tell you what the behavior of a program should be. That's undefined behavior. — Pete Becker, Apr 05 '22 at 13:30
There more mistakes in this code (memory leak since there is no deallocation by smart pointer or destructor). What is desired functionality (since current code is a bit weird). — Marek R, Apr 05 '22 at 13:44
O, I didn't notice that the destructor of your class is missing, I just assumed it's there and it is deleting the memory. Is it intentional, or you just forgot to copy-paste your full class body? — pptaszni, Apr 05 '22 at 13:45

Adrian Mole · Accepted Answer · 2022-04-05T23:02:35.057

Your call to make_shared is using a copy constructor for your Foo class, which you haven't defined. Thus, the default (compiler-generated) copy will be used, and the destructor will be called to delete the temporary. As your class doesn't properly implement the Rule of Three, this (potentially) causes undefined behaviour.

The copy constructor is being used because the argument list in your call to std::make_shared is Foo(3, false) – a Foo object, so the call fits only the first overload listed on this cppreference page. (Note that there is nothing resembling a std::make_shared<T>(const T& src) overload.) From that page, we see:

Constructs an object of type T and wraps it in a std::shared_ptr using args as the parameter list for the constructor of T.

So, with your "args" of Foo(3, false), that make_shared actually calls the following constructor for the object to wrap:

Foo(Foo(3, false))

For correct behaviour, just pass the 3 and false as arguments to make_shared:

std::shared_ptr<Foo> sp_foo = std::make_shared<Foo>(3, false);

You can demonstrate the error in your original code by adding a destructor to the Foo class that logs some output, like: ~Foo() { std::cout << "Destroying...\n"; }. You will see that output after execution of the call to make_shared.

You can prevent this accidental error/oversight by deleting the Foo copy constructor: Foo(const Foo& f) = delete;. This will generate a compiler error along the following lines:

error : call to deleted constructor of 'Foo'

However, in your second case, you use a std::shared_ptr constructor (the third form shown on the linked page). This has no problem, because that constructor 'just' wraps the given pointer into the managed object, and no copying or destruction is needed.

As the class doesn't have a destructor `arr_` isn't deleted so there isn't any undefined behaviour from the copied object — Alan Birtles, Apr 05 '22 at 22:07
@AlanBirtles Yeah - I added "(potentially)" to the undefined behaviour part. But a destructor or a copy c'tor would be very difficult, anyway: `delete [] p;` on a `void*` is UB according to clang: *warning : cannot delete expression with pointer-to-'void' type 'void *' [-Wdelete-incomplete]*. And copying data from a `void*`, not knowing its size or underlying type will be *very* tricky. — Adrian Mole, Apr 05 '22 at 23:05

std::make_shared leads to undefined behavior, but new works

1 Answers1