Making a safe buffer holder in C++

user_10116440 0
Viewed 1349

There are situations in which I need to pass a char* buffer back and forth. My idea is to create an object which can hold the object that owns the data, but also expose the data as char* for someone to read. Since this object holds the owner, there are no memory leaks because the owner is destructed with the object when it's no longer necessary.

I came with the implementation below, in which we have a segfault that I explain why it happens. In fact it's something that I know how to fix but it's something that my class kinda lured me into doing. So I consider what I've done to be not good and maybe there's a better way of doing this in C++ that is safer.

Please take a look at my class that holds the buffer owner and also holds the raw pointer to that buffer. I used GenericObjectHolder to be something that holds the owner for me, without my Buffer class being parametrized by this owner.

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

//The library:
class GenericObjectHolder
{
public:
    GenericObjectHolder()
    {
    }

    virtual ~GenericObjectHolder() {
        
    };
};

template <class T, class Holder = GenericObjectHolder>
class Buffer final
{
public:
    //Ownership WILL be passed to this object
    static Buffer fromOwned(T rawBuffer, size_t size)
    {
        return Buffer(std::make_unique<T>(rawBuffer), size);
    }

    //Creates a buffer from an object that holds the buffer
    //ownership and saves the object too so it's only destructed
    //when this buffer itself is destructed
    static Buffer fromObject(T rawBuffer, size_t size, Holder *holder)
    {
        return Buffer(rawBuffer, std::make_unique<T>(rawBuffer), size, holder);
    }

    //Allocates a new buffer with a size
    static Buffer allocate(size_t size)
    {
        return Buffer(std::make_unique<T>(new T[size]), size);
    }

    ~Buffer()
    {
        if (_holder)
            delete _holder;
    }

    virtual T data()
    {
        return _rawBuffer;
    }

    virtual size_t size() const
    {
        return _size;
    }

    Buffer(T rawBuffer, std::unique_ptr<T> buffer, size_t size)
    {
        _rawBuffer = rawBuffer;
        _buffer = std::move(buffer);
        _size = size;
    }

    Buffer(T rawBuffer, std::unique_ptr<T> buffer, size_t size, Holder *holder)
    {
        _rawBuffer = rawBuffer;
        _buffer = std::move(buffer);
        _size = size;
        _holder = holder;
    }


    Buffer(const Buffer &other)
        : _size(other._size),
          _holder(other._holder),
          _buffer(std::make_unique<T>(*other._buffer)) 
    {
    }


private:
    Holder *_holder;
    T _rawBuffer;
    std::unique_ptr<T> _buffer;
    size_t _size = 0;
};

//Usage:
template <class T>
class MyHolder : public GenericObjectHolder
{
public:
    MyHolder(T t) : t(t)
    {
    }

    ~MyHolder()
    {
    }

private:
    T t;
};

int main()
{
    std::queue<Buffer<const char*, MyHolder<std::string>>> queue;
    std::cout << "begin" << std::endl;
    {
        //This string is going to be deleted, but `MyHolder` will still hold
        //its buffer
        std::string s("hello");
        auto h = new MyHolder<std::string>(s);
    
        auto b = Buffer<const char*, MyHolder<std::string>>::fromObject(s.c_str(),s.size(), h);
        queue.emplace(b);
    }
    {
        auto b = queue.front();
        //We try to print the buffer from a deleted string, segfault
        printf(b.data());
        printf("\n");
    }
    std::cout << "end" << std::endl;
}

As you see, the s string is copied inside the object holder but gets destructed right after it. So when I try to access the raw buffer that buffer owns I get a segfault.

Of course I could simply copy the buffer from the s string into a new buffer inside my object, but It'd be inefficient.

Maybe there's a better way of doing such thing or maybe there's even something ready in C++ that does what I need.

PS: string is just an example. In pratice I could be dealing with any type of object that owns a char* buffer.

Live example: https://repl.it/repls/IncredibleHomelySdk

memory-leaks memory-management c++
3 Answers

Your core problem is that you want your Holder to be moveable. But when the Owner object moves, the buffer object might also move. That will invalidate your pointer. You can avoid that by putting the owner in a fixed heap location via unique_ptr:

#include <string>
#include <memory>
#include <queue>
#include <functional>

template <class B, class Owner>
class Buffer
{
public:
    Buffer(std::unique_ptr<Owner>&& owner, B buf, size_t size) :
      _owner(std::move(owner)), _buf(std::move(buf)), _size(size)
    {}

    B data() { return _buf; }
    size_t size() { return _size; }

private:
    std::unique_ptr<Owner> _owner;
    B _buf;
    size_t _size;
};

//Allocates a new buffer with a size
template<typename T>
Buffer<T*, T[]> alloc_buffer(size_t size) {
    auto buf = std::make_unique<T[]>(size);
    return {std::move(buf), buf.get(), size};
}

Here's a repl link: https://repl.it/repls/TemporalFreshApi

If you want to have a type-erased Buffer, you can do that like this:

template <class B>
class Buffer
{
public:
    virtual ~Buffer() = default;
    B* data() { return _buf; }
    size_t size() { return _size; }

protected:
    Buffer(B* buf, size_t size) : 
      _buf(buf), _size(size) {};
    B* _buf;
    size_t _size;
};


template <class B, class Owner>
class BufferImpl : public Buffer<B>
{
public:
    BufferImpl(std::unique_ptr<Owner>&& owner, B* buf, size_t size) :
      Buffer<B>(buf, size), _owner(std::move(owner))
    {}
    
private:
    std::unique_ptr<Owner> _owner;
};

//Allocates a new buffer with a size
template<typename T>
std::unique_ptr<Buffer<T>> alloc_buffer(size_t size) {
    auto buf = std::make_unique<T[]>(size);
    return std::make_unique<BufferImpl<T, T[]>>(std::move(buf), buf.get(), size);
}

Again, repl link: https://repl.it/repls/YouthfulBoringSoftware#main.cpp

user_7583450

You wrote:

There are situations in which I need to pass a char* buffer back and forth.

and

So I consider what I've done to be not good and maybe there's a better way of doing this in C++ that is safer.

It's not exactly clear what you are aiming at, but when I have this need i will sometimes use std::vector<char> - a std::vector (and std::string) is a just that: a managed buffer. Calling data() on vector will give you a raw pointer to the buffer to pass on to legacy interfaces etc. or for whatever reason you just need a buffer that you manage yourself. Hint: use resize() or constructor to allocate the buffer.

So you see, there's no need to store the internal pointer of std::string in your example. Instead just call data() on a need basis.

It seems like you are concerned about copies and efficiency. If you use objects that support move semantics and you use the emplace family of functions there shouldn't be any copy-ing going on at least in c++17. All/most containers supports moving as well.

user_22615380

The class std::unique_ptr is already a "buffer holder" that "guarantee delete", no string copies, no dangling references and no seg faults:

#include <iostream>
#include <queue>
#include <memory>

int main()
{
    std::queue<std::unique_ptr<std::string>> queue;
    std::cout << "begin" << std::endl;
    {
        auto h = std::make_unique<std::string>("Hello");
        queue.emplace( std::move(h) ); // move into the queue without copy
    }
    {
        auto b = std::move(queue.front());  // move out from queue without copy
        std::cout << *b << std::endl;
    }  // when b goes out of scope it delete the string
    std::cout << "end" << std::endl;
}

https://godbolt.org/z/neP838

user_56055740
Related