CODEWITHSUNDEEP
c++shared-ptrvalue-type
marczellm
marczellm

Reputation: 1336

Can you use a member of deleter to keep alive a shared_ptr?

Given the following types

// interface and implementation used in one part of the codebase
struct Image
{
    virtual std::vector<uint8_t>& GetData () = 0;
};

struct VecImage : public Image
{
    std::vector<uint8_t> mData;

    std::vector<uint8_t>& GetData () { return mData; }
};

// used in another part of the codebase
struct PtrImage
{
    std::shared_ptr<uint8_t> mData;

    PtrImage (std::shared_ptr<Image> pIm);
};

is the following constructor a sane and correct way to convert an Image to a PtrImage?

PtrImage::PtrImage (std::shared_ptr<Image> pIm)
{
    struct im_deleter
    {
        std::shared_ptr<Image> keepAlive;
        void operator () (uint8_t* ptr)
        {
            keepAlive.reset ();
        }
    };

    mData = { &pIm->GetData()[0], im_deleter { pIm } };
}

PtrImage is used as a "value type", it is being passed around by value, while Image is passed around in shared_ptrs only.

Upvotes: 2

Views: 130

Answers (3)

Aconcagua
Aconcagua

Reputation: 25526

Looks pretty dangerous to me:

std::shared_ptr<Image> i = std::make_shared<VecImage>(/* some data */);
PtrImage p(i); // has now stored a pointer to the vector's data
i->getData()->push_back(0); // repeat until re-allocation occurs!

What would p now hold? The shared pointer holds a pointer to the data that resided in the vector before re-allocation; but this data was replaced and got deleted. So you now have a dangling pointer stored in p (in the uint8_t pointer), using it (which will happen at latest when your smart pointer tries to delete its data) will result in undefined behaviour.

Upvotes: 1

Jarod42
Jarod42

Reputation: 217283

is the following constructor a sane..

You extend lifetime of Image thanks to destructor, so data is still valid. So you are correct on that point...

But, vector may reallocate, invalidating the buffer.

So resulting code is unsafe.

You could store std::shared_ptr<std::vector<uint8_t>> mData; to be safe.

.. and correct way

We have better/simpler with aliasing constructor of std::shared_ptr:

struct PtrImage
{
    std::shared_ptr<std::vector<uint8_t>> mData;

    PtrImage (std::shared_ptr<Image> pIm) : mData(pIm, &pIm->GetData()) {}
};

So ownership information PtrImage::mData is shared with pIm.

Note: I assumes that vector returned by GetData() has same (or longer) lifetime that Image (as for VecImage). if it is an unrelated vector (from other object), then you won't have solution.

As noted in comment, vector should not reallocate neither

Upvotes: 3

Serge Ballesta
Serge Ballesta

Reputation: 148910

You should not even try to have a shared pointer guess whether it should delete its object or not. It you proceed that way, you will be caught at a time by a corner case or even if you managed to find all, by a new one created by an apparently unrelated change.

If you need to convert an Image to a PtrImage just say that you want to build a shared_ptr<T> from a T. There are 2 standard ways: a copy or a move, and in either the shared_ptr has ownership of its object.

In your example, as Image only returns a lvalue reference, you can only use a copy. You could have a move from a VecImage by taking ownership of its data member.

Upvotes: 0

Related Questions