unique_ptr read access violation when losing scope

Question

I'm reading a char array from a binary file using std::ifstream, then reinterpreting the char array into a pointer to my struct then making this a unique_ptr. Everything works well except when the unique_ptr goes out of scope I get a read access violation.

Am I going about this all wrong? I'm not sure why the error is occurring. I only have one unique_ptr on that data.

I've included the very basic version of the code to produce the error.

struct mystruct {
    int val = 0;
    double val2 = 5.6;
    std::string string = "test";

};

int main()
{

    //brackets here just to encapsulate the scope for this example
    {
        //try to open the stream for reading
        std::ifstream stream;
        stream.open("test.bin", std::ios::binary | std::ios::in);

        char* buffer = new char[sizeof(mystruct)];
        stream.read(buffer, sizeof(mystruct));

        mystruct * pointer = reinterpret_cast<mystruct*>(buffer);
        std::unique_ptr<mystruct> obj = std::unique_ptr<mystruct>(pointer);

        //ha no problem reading the struct data
        std::cout << "read back: " << obj->string << endl;

        stream.close();
    }

    //obj goes out of scope and throws a read access violation
}

I expect the unique_ptr to just delete the object and no error should be thrown

**********EDIT********************
Thanks for the comments and answers - basically from your help I produced the code that I was trying to do so have listed it here in case it helps anyone else.
Main Points were:
* std::string is not recommended in struct if reading and writing from binary because std::string has unknown number of bytes.
* needed to create the object in memory before assigning the pointer to it - std::make_unique() was suitable for this.

struct mystruct {
    int val1 = 0;
    double val2 = 5.6;
    char somestring[10] = "string";

};

int main()
{

    //brackets here just to encapsulate the scope for this example
    {
        //try to open the stream for reading
        std::ifstream stream;
        stream.open("test.bin", std::ios::binary | std::ios::in);

        //hold the results in a vector
        auto results = std::vector<std::unique_ptr<mystruct>>();

        //read a vectory or mystructs from the binary file
        while (!stream.eof())
        {
            //create the object - NOTE: make_unique initialises my struct
            std::unique_ptr<mystruct> obj = std::make_unique<mystruct>();

            //read from binary file into obj
            if (!stream.read(reinterpret_cast<char*>(obj.get()), sizeof mystruct))
                break;

            //add the obj to th vector
            results.push_back(std::move(obj));
        }

        stream.close();

        for (auto& val : results)
        {
            cout << "read back: " << val->somestring << endl;
        }


    }
}

Your program exhibits undefined behavior, by way of passing to `delete` a pointer of a type different than that allocated by `new`. Actually, it exhibits undefined behavior earlier, at `obj->string`, by way of accessing an object before its lifetime has started; you just happen to get away with it (probably thanks to small-string optimization). — Igor Tandetnik, May 17 '19 at 03:39
Is there a better way to read a binary file into a struct with a unique_ptr? — Matthew Bailey, May 17 '19 at 03:41
Assuming no class-specific `operator new`, use `::operator new` to do raw allocations, then use placement new. — o11c, May 17 '19 at 03:41
Your struct contains a `std::string` which isn't [POD](https://stackoverflow.com/questions/146452/what-are-pod-types-in-c), so writing and reading with `fstream::write()` and `fstream::read()` won't work. — Swordfish, May 17 '19 at 03:41
But you definitely can't read a `std::string` from a file, that doesn't make sense. — o11c, May 17 '19 at 03:41
You can't write a non-POD struct (e.g. one that contains a `std::string` member) to a file this way to begin with. — Igor Tandetnik, May 17 '19 at 03:41
Ahh, that makes sense. Deleting the string from the struct makes everything work. — Matthew Bailey, May 17 '19 at 03:48
@MatthewBailey `stream.read(buffer, sizeof(mystruct));` -- This should have been the clue that this could never work, due to `sizeof(mystruct)` being the same value, regardless of how many characters you have / had in your `std::string`. It wouldn't make sense to read only (for example) 32 or 48 bytes of data from a file, each and every time. — PaulMcKenzie, May 17 '19 at 03:53
Makes sense now about the string (should have thought of that). This post here actually helped read from the binary data properly in the first place. https://stackoverflow.com/questions/56041755/read-binary-bytes-beginning-at-position-n-within-ifstream?rq=1 — Matthew Bailey, May 17 '19 at 04:02
A stream is a stream of bytes. If you want to write objects to, and read objects from, a stream, you have to convert them to and from streams of bytes. You have no code to do that here. — David Schwartz, May 17 '19 at 04:45

Nicol Bolas · Accepted Answer · 2019-05-17T03:48:12.533

There are 3 kinds of undefined behavior in your code.

First, you pretend there's an object in a place that there isn't one. You never created a mystruct; you just allocated some bytes of memory. Merely doing a reinterpret_cast isn't sufficient to create a mystruct. So any use of pointer that accesses the "object" that doesn't exist is UB.

Second, even if there was one in that buffer, mystruct is not trivially copyable, so you cannot just copy its bytes around. You can't read a bunch of bytes into a non-trivially copyable object. It is the presence of a non-trivially copyable non-static data member (ie: mystruct::string) that makes it non-trivially copyable.

And third, you try to delete this mystruct. But there there is no mystruct, you're deleting a thing that doesn't exist. Technically, that's probably covered by #1, but it's likely what's causing your code to outright crash.

If you to know why "no problem reading the struct data" happened to work, odds are good that the std::string implementation is using small string optimization, which stores the string inside the std::string itself if it is small enough. For small strings, doing a bytewise copy is probably close enough to "working" to allow you to read the string data.

But that's just getting lucky.

unique_ptr read access violation when losing scope

1 Answers1