1

Why does std::move() cause a SEGFAULT in this case?

#include <iostream>

struct Message {
    std::string message;
};

Message * Message_Init(std::string message) {
    Message * m = (Message*)calloc(1, sizeof(Message));
    m->message = std::move(message);
    return m;
}

int main() {
    auto m = Message_Init("Hello");
    return 0;
}

P.S. Please don't ask why Message is not constructed in a usual C++ manner.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 3
    The pointed message *m is not constructed at all, in either method. – 273K Apr 27 '22 at 06:28
  • 1
    You call the move assignment operator with the left hand side uninitialized. – Jakob Stark Apr 27 '22 at 06:33
  • @273K So std::move is **not** just move std::string structure to a new place? – Евгений Павлов Apr 27 '22 at 06:34
  • 4
    The `malloc` function allocates memory, but nothing more. The constructor of `Message` is not called, and therefore the string `message` will also be unconstructed. When you try to move the string object you will have *undefined behavior*. – Some programmer dude Apr 27 '22 at 06:35
  • 2
    A lot of C++ classes in the std:: library can't just be "zero'd out" with calloc or memset and expect to function. These classes have constructors that setup valid pointers. For those pointer to not be there when a move into operation occurs creates undefined behavior. – selbie Apr 27 '22 at 06:35
  • 2
    @ЕвгенийПавлов No it inspects the target string object to find out the most efficient ways to move the contents. If that object is invalid, anything may happen. – Jakob Stark Apr 27 '22 at 06:36
  • 2
    Generally speaking, if you even feel the need to use a C-style cast (like `(Message*)`) then you should take that as a sign that you're doing something wrong. – Some programmer dude Apr 27 '22 at 06:36
  • Another general tip: Unless you have polymorphic classes, you don't really need pointers in modern C++. In this case, pointers and dynamic allocation doesn't seem to be needed at all. – Some programmer dude Apr 27 '22 at 06:37
  • @Someprogrammerdude Ok. Thanks, I think it's better for me to switch to plain C. – Евгений Павлов Apr 27 '22 at 06:44
  • 2
    The message has allocated space but you never initialised it (call to ctor). You can use the **placement-new operator**, something like `char *p = malloc(...); Message *m = new ( p ) Message();` – Jean-Baptiste Yunès Apr 27 '22 at 06:46
  • @ЕвгенийПавлов *Please don't ask why Message is not constructed in a usual C++ manner.* -- No need to ask, because `Message` was never constructed. As others pointed out, all you did was allocate a blob of bytes -- you didn't create a `Message` object. – PaulMcKenzie Apr 27 '22 at 08:01

2 Answers2

6

If you really want to do something like this, then you can use placement new. This allows you to construct an object in a chunk of memory that is already allocated.

Several of the standard containers use placement new to manage their internal buffers of objects.

However, it does add complications with the destructors of the objects you place. Read here for more info: What uses are there for "placement new"?

#include <iostream>
#include <memory>

struct Message {
    std::string message;
};

Message * Message_Init(std::string message) {
    void * buf = calloc(1, sizeof(Message));
    Message * m = new (buf) Message(); // placement new
    m->message = std::move(message);
    return m;
}

int main() {
    auto m = Message_Init("Hello");
    m->~Message();  // Call the destructor explicitly
    free(m);        // Free the memory
    return 0;
}

As @selbie suggested, I have added an explicit call to the destructor of Message, and also a call to free to deallocate the memory. I believe that the call to free should actually have pointed to the buffer originally returned by calloc since there could be a difference (so free(buf) in this case, but that pointer is not accessible here).

For example if you allocate a buffer for several objects, then calling free on the pointer to the second object would not be correct.

Frodyne
  • 3,547
  • 6
  • 16
  • 1
    You should probably call out the need to have the destructor of Message explicitly called. Also, no need to invoke std::move. If the function was declared as `Message* Message_Init(const std::string& message`, then the assignment can be simply `m->message = message` to avoid a redundant copy. – selbie Apr 27 '22 at 06:56
0

There seems to be a misunderstanding about how assignment in C++ works. If the compiler encounters a assigment operation with class types involved, it looks for a special function called the copy/move assignment operator. This special function is then called to perform the copy/move. Generally speaking the left hand side and the right hand side have to be initialized objects for this to work correctly.

There is a special case in which it is allowed to use uninitialized objects. That is the case where the class of the objects that are to be copied/moved has a trivial copy/move assigment operator. However most of the standard library classes are not trivially copiable/moveable and in particular std::string is not.

By the way it does not matter that you called std::move. Even without it, it would be undefined behaviour. You have to construct the target object before assigning to it. You can do that by either allocating and constructing the object in one shot by calling the normal new operator, or by allocating the space and use placement new to construct the object in that space.

Jakob Stark
  • 3,346
  • 6
  • 22