C++ object containing an array of char using unique_ptr

Question

I am looking on a way to use unique_ptr to allocate a structure that contains an array of char with a number of bytes that set dynamically to support different types of message.

Assuming:

struct MyMessage
{
    uint32_t      id;
    uint32_t      data_size;
    char          data[4];
};

How can I convert send_message() below to use a smart pointer?

void send_message(void* data, const size_t data_size)
{
    const auto message_size = sizeof(MyMessage) - 4 + data_size;
    const auto msg = reinterpret_cast<MyMessage*>(new char[message_size]);

    msg->id = 3;
    msg->data_size = data_size;
    memcpy(msg->data, data, data_size);

    // Sending the message
    // ...

    delete[] msg;
}

My attempt to use smart point using the code below does not compile:

const auto message_size = sizeof(MyMessage) - 4 + data_size;
const auto msg = std::unique_ptr<MyMessage*>(new char[message_size]);

Below a complete working example:

#include <iostream>
#include <iterator>
#include <memory>

using namespace std;

struct MyMessage
{
    uint32_t      id;
    uint32_t      data_size;
    char          data[4];
};

void send_message(void* data, const size_t data_size)
{
    const auto message_size = sizeof(MyMessage) - 4 + data_size;
    const auto msg = reinterpret_cast<MyMessage*>(new char[message_size]);
    if (msg == nullptr)
    {
        throw std::domain_error("Not enough memory to allocate space for the message to sent");
    }
    msg->id = 3;
    msg->data_size = data_size;
    memcpy(msg->data, data, data_size);

    // Sending the message
    // ...

    delete[] msg;
}

struct MyData
{
    int  page_id;
    char point_name[8];
};

void main()
{
    try
    {
        MyData data{};
        data.page_id = 7;
        strcpy_s(data.point_name, sizeof(data.point_name), "ab332");
        send_message(&data, sizeof(data));
    }
    catch (std::exception& e)
    {
        std::cout << "Error: " << e.what() << std::endl;
    }
}

Your code as is contains undefined behavior and if it works you are lucky (actually unlucky as working makes you think it is okay). `const auto msg = reinterpret_cast(new char[message_size]);` violates strict aliasing and `memcpy(msg->data, data, data_size);` goes out of bounds of the array `data` for any `data_size > 4`. — NathanOliver, May 02 '18 at 20:27
This technique is broadly used in our company and I would think if you look again you may see that msg->data points to an offset of the memory that has been allocated by new char[message_size]. It may look there is a bound of the array but that array is indeed accessing a location in memory that contains the relevant size. Does it make sense to you? — Less White, May 02 '18 at 20:40
@LessWhite unfortunately even if it is used broadly by a company that would not make this code legal C++ — Slava, May 02 '18 at 20:42
I get it but it is still undefined behavior according to the standard. `data` is only 4 bytes wide so anything more than that is accessing the array out of its bounds which is UB. It doesn't matter if there is still more valid storage past the array. — NathanOliver, May 02 '18 at 20:42
I don't know how to do better in a sense that I don't want to declare a char* for msg->data since a pointer won't be referred properly by the destination application receiving the data. Casting it to that structure is a trick that seems to work but I am willing to fix it if I can find a better way. Thanks for your help. — Less White, May 02 '18 at 20:57
Do both your sender and receiver have the definition of the struct in a header for example? — Vamsidhar Reddy Gaddam, May 02 '18 at 21:07
@LessWhite What about something like this: http://coliru.stacked-crooked.com/a/31cb72051dce3c8a — NathanOliver, May 02 '18 at 21:08
It looks slightly sensible, but the whole thing (the original post mostly) and the solution feels like over engineering to fit C and C++ ideas into one place. why not use strings? Is it because they are used in some threads? I guess I am missing the point here. — Vamsidhar Reddy Gaddam, May 02 '18 at 21:13
Answering your main question, you can do this: const auto msg = std::unique_ptr(reinterpret_cast(new char[message_size])); But you will have an indeterminate behavior in the delete made for unique_ptr. — rflobao, May 02 '18 at 21:15
@BenVoigt is accessing `data[]` out of range going to be ok as well? — Slava, May 02 '18 at 21:20
@BenVoigt I know what it is, that still does not make it legal C++ — Slava, May 02 '18 at 21:29
oops, misremembered the rule. Pointer comparison will work within a single allocation, but pointer arithmetic won't (can't cross member boundaries even to point to other parts of the same complete object) — Ben Voigt, May 02 '18 at 21:35
@rflobao the "indeterminate behavior" can be fixed by giving the `unique_ptr` a custom `deleter` that casts the `Message*` pointer back to `char*` and then frees it with `delete[]` instead of the default `delete`. — Remy Lebeau, May 02 '18 at 21:58

Remy Lebeau · Accepted Answer · 2018-05-03T16:31:37.683

The data type that you pass to delete[] needs to match what new[] returns. In your example, you are new[]ing a char[] array, but are then delete[]ing a MyMessage object instead. That will not work.

The simple fix would be to change this line:

delete[] msg;

To this instead:

delete[] reinterpret_cast<char*>(msg);

However, You should use a smart pointer to manage the memory deletion for you. But, the pointer that you give to std::unique_ptr needs to match the template parameter that you specify. In your example, you are declaring a std::unique_ptr whose template parameter is MyMessage*, so the constructor is expecting a MyMessage**, but you are passing it a char* instead.

Try this instead:

// if this struct is being sent externally, consider
// setting its alignment to 1 byte, and setting the
// size of the data[] member to 1 instead of 4...
struct MyMessage
{
    uint32_t      id;
    uint32_t      data_size;
    char          data[4];
};

void send_message(void* data, const size_t data_size)
{
    const auto message_size = offsetof(MyMessage, data) + data_size;

    std::unique_ptr<char[]> buffer = std::make_unique<char[]>(message_size);
    MyMessage *msg = reinterpret_cast<MyMessage*>(buffer.get());    

    msg->id = 3;
    msg->data_size = data_size;
    std::memcpy(msg->data, data, data_size);

    // Sending the message
    // ...
}

Or this:

using MyMessage_ptr = std::unique_ptr<MyMessage, void(*)(MyMessage*)>;

void send_message(void* data, const size_t data_size)
{
    const auto message_size = offsetof(MyMessage, data) + data_size;

    MyMessage_ptr msg(
        reinterpret_cast<MyMessage*>(new char[message_size]),
        [](MyMessage *m){ delete[] reinterpret_cast<char*>(m); }
    );

    msg->id = 3;
    msg->data_size = data_size;
    std::memcpy(msg->data, data, data_size);

    // Sending the message
    // ...
}

Actually changing `data` array size to 1 most probably will make it to waste 3 bytes additionally, not safe. — Slava, May 03 '18 at 02:58
@Slava unless structure alignment is set to 1 byte, which would make sense when sending struct data externally. — Remy Lebeau, May 03 '18 at 05:07
Thanks Remy, your code is awesome and works great. It helps me so much understanding how to use smart pointers. — Less White, May 03 '18 at 13:37
Then you clearly need to say "disabling alignment is required" otherwise you have the same issue with `message_size` as me or even worse (as it is pretty much hidden) — Slava, May 03 '18 at 13:51
@Slava I changed the code to restore the original struct, with comment. And to use `offsetof()` instead of `sizeof()` — Remy Lebeau, May 03 '18 at 16:32

Slava · Answer 2 · 2018-05-03T13:48:19.900

0

This should work, but it is still not clear if accessing msg->data out of bounds is legal (but at least it is not worst than in your original code):

const auto message_size = sizeof(MyMessage) - ( data_size < 4 ? 0 : data_size - 4 );
auto rawmsg = std::make_unique<char[]>( message_size );
auto msg = new (rawmsg.get()) MyMessage;

edited May 03 '18 at 13:48

answered May 02 '18 at 21:32

Slava

43,454
1
47
90

Related: [C++ lacks flexible array members](https://stackoverflow.com/questions/4412749/are-flexible-array-members-valid-in-c) – HolyBlackCat May 02 '18 at 21:34
This "works" , but only legally if `message_size` is at least `sizeof(MyMessage)` (which means the `data_size` must be >= 4). It also means the code now has a `std::unique_ptr` instead of a `std::unique_ptr`, if the message needs to be passed around to other functions. – Remy Lebeau May 02 '18 at 22:00
It looks to me that after having processed msg I must call delete msg to prevent a memory leak (which defeat my purpose to use smart pointer to avoid memory leaks). In that sense, this solution does not bring me any benefit. – Less White May 02 '18 at 22:11
@LessWhite no, you would not need to call `delete msg`. The destructor of `rawmsg` will call `delete[]` for you. If anything, when using **placement-new** to construct an object, you have to call the object's destructor manually, ie `msg->~MyMessage()`, however that is optional in this situation because `MyMessage` only contains POD types that don't need to be destructed. Things would be different if `MyMessage` contained non-POD members – Remy Lebeau May 02 '18 at 22:33
@RemyLebeau `message_size` in OP's code is at least `sizeof(MYMessage)` I assume rest of code does not change – Slava May 03 '18 at 02:56
@Slava `message_size < sizeof(MyMessage)` if `data_size < sizeof(MyMessage::data)`. That is not the case for `MyData` but maybe the OP has other messages with smaller data, we don't know. – Remy Lebeau May 03 '18 at 05:03
@RemyLebeau got it, fixed size in the answer. Your code actually has the same issue, but even worse, as it is hidden :) – Slava May 03 '18 at 13:49
@Slava my code doesn't construct an `MyMessage` object so having a proper size is not as important. Your code is constructing an object, so size matters. – Remy Lebeau May 03 '18 at 16:35

C++ object containing an array of char using unique_ptr

2 Answers2