How to write move constructor to handle uninitialized move?

Question

I have a class in my C++ code which has its own move constructor. A simplified version is shown here:

class myClass {
    //In this example, myClass must manually manage allocating
    //and freeing a memory buffer.
    char *mem;

    //...
    //Regular constructor, copy constructor, etc
    //...

    myClass(myClass &&other) {
        //Swap our memory pointer with other's memory pointer
        char *tmp = other.mem;
        other.mem = mem;
        mem = tmp;
    } 

    //...
    //Destructor, other member functions, etc.
    //...
}

In normal situations, this works fine. However, recently I needed to make a vector of these objects:

vector<myClass> v;
v.reserve(10); //Make space, but do not construct
v.push_back(myClass()); //Problem!

After getting a segfault and stepping through with gdb, I eventually discovered what should have been obvious: if you try to construct an object from an rvalue reference, this can result in using the move constructor on uninitialized memory.

How are you supposed to write a move constructor when it's possible that you're swapping garbage into the other class? Is there some way to detect this?

Do you need a memory buffer, or do you really need `std::string`? — tadman, May 19 '19 at 00:38
I simplified this example for the sake of asking a question on a forum. In rare cases you really do want to manage your own memory. In any case, how does std::string solve this problem? — Marco Merlini, May 19 '19 at 00:42
I think you need to show us the regular no-argument constructor. Presumably it initialises `mem`, if only to `nullptr`? (Also, there is no reason for `other.mem = mem` in your copy constructor; better is `free (mem); mem = other.mem; other.mem = nullptr;`) — TonyK, May 19 '19 at 00:45
@TonyK I think doing a `free(other.mem)` should work actually (instead of copying). And by the way, my default constructor does give `mem` a reasonable default value; in my case, I was using `std::vector::reserve`, which wasn't default-constructing the memory (I made an edit to the question to show this). — Marco Merlini, May 19 '19 at 00:49
When I see `char*` pointers being passed around like candy I immediately suspect that there's some serious memory management (double `free` or no `free` at all which leaks) issues here or uninitialized pointer data to blame for the problem. `std::string` keeps things simple. — tadman, May 19 '19 at 00:52
@TonyK This doesn't completely solve the problem. `free(mem)` is still potentially freeing a garbage pointer. — Marco Merlini, May 19 '19 at 00:52
A constructor **always** gets applied to uninitialized memory. So don’t swap the pointer. Just copy it to the object being constructed and set the moved-from pointer to 0. — Pete Becker, May 19 '19 at 00:53
Related: [What is The Rule of Three?](https://stackoverflow.com/q/4172722) and [What is the copy-and-swap idiom?](https://stackoverflow.com/q/3279543) — L. F., May 19 '19 at 00:53
@PeteBecker You can call a constructor on initialized memory. In fact, this is what I had been doing in the past, which is why I never realized this problem until today. Anyway, I've learned something: don't write move constructors like a move assignment operator — Marco Merlini, May 19 '19 at 00:58
@Mahkoe Calling a constructor on initialized memory is undefined behavior. You are accessing the value of an uninitialized pointer (`mem`). — L. F., May 19 '19 at 01:00
@Mahkoe — a constructor constructs an object where there was not one before. It cannot rely on any particular values being in the memory that the constructor is working on. If your constructor happens to work because of some pattern of values that happens to occur in memory that’s just bad luck. — Pete Becker, May 19 '19 at 02:25
It would improve the question to include a MCVE, or failing that, clarify whether `other.mem` might be uninitialized (or what you meant by "uninitialized move", if it isn't). — M.M, May 19 '19 at 04:24
"Anyway, I've learned something: don't write move constructors like a move assignment operator": that is not the lesson that you should be taking away from this. The lesson is "if your class contains a pointer, initialise it to zero in the default constructor." — TonyK, May 19 '19 at 09:08

Michael Kenzel · Answer 1 · 2019-05-19T01:31:09.753

How are you supposed to write a move constructor when it's possible that you're swapping garbage into the other class? Is there some way to detect this?

An object that is not initialized holds an indeterminate value until assigned another value [basic.indet]/1. You're basically not allowed to do anything with an object holding an indeterminate value except for assigning it a proper value [basic.indet]/2. Since you're not even allowed to look at the value an object holds unless it has been initialized or assigned a value, there cannot possibly be a way to detect whether an object has been initialized just by looking at the object itself (because you're not allowed to even take a look). Thus, strictly speaking, you're actually not just "swapping garbage values into the other class", you're invoking undefined behavior. Garbage being swapped is just how that undefined behavior will typically manifest.

The solution to the problem is simple: Make sure that your pointer is always initialized to a valid value, e.g., nullptr:

class myClass {
    //In this example, myClass must manually manage allocating
    //and freeing a memory buffer.
    char *mem = nullptr;

    //...
    //Regular constructor, copy constructor, etc
    //...

    myClass(myClass &&other) {
        //Swap our memory pointer with other's memory pointer
        char *tmp = other.mem;
        other.mem = mem;
        mem = tmp;
    } 

    //...
    //Destructor, other member functions, etc.
    //...
}

Rather than implement the move constructor yourself, consider, e.g., just using a member of type std::unique_ptr and simply relying on the implicitly defined move constructor. For example:

class myClass
{
    std::unique_ptr<char[]> mem;

    // regular constructor, copy constructor, etc.

    myClass(myClass&&) = default;

    // other member functions, etc.
};

Using the mem-initializer and `std::exchange()` is also interesting. Or maybe copy-and-swap. Of course, following the rule-of-zero like you show is best, though why you note down a copy-ctor to be deleted... — Deduplicator, May 19 '19 at 01:20
@Deduplicator yes, I'm not sure what I was thinking about that copy constructor, thank's for pointing that out…fixed…I guess it's time to catch some sleep… — Michael Kenzel, May 19 '19 at 01:25
Why bother with the swap at all? Just initialize the member in the move constructor's member initializer, and null out the other object's pointer in the constructor. — Nicol Bolas, May 19 '19 at 01:38

Nicol Bolas · Answer 2 · 2019-05-19T01:49:27.650

3

Don't swap the pointers in the constructor. That's not how you write move constructors. Swapping is for move-assignment, when both objects are live.

Constructors exist to initialize an object. As such, the memory they start with is always in the "uninitialized" state. So unless you initialize a member (or it has a default constructor that initializes it for you), the member's value will start uninitialized.

The correct way to handle this is just copy the pointer in the member initializer, then null out the other one.

myClass(myClass &&other) : mem(other.mem) {
    other.mem = nullptr;
}

Or, with C++14 (and C++20 with a constexpr version), you can exchange the value:

myClass(myClass &&other)
  : mem(std::exchange(other.mem, nullptr))
{}

edited May 19 '19 at 01:49

answered May 19 '19 at 01:39

Nicol Bolas

449,505
63
781
982

1

A slightly more compact way to do the same thing (as also suggested by @Deduplicator), would be to just use `std::exchange(other.mem, nullptr)` to initialize `mem`. – Michael Kenzel May 19 '19 at 01:47
@MichaelKenzel: Not if you need your constructor to be `constexpr`. C++20 fixes that issue, though. – Nicol Bolas May 19 '19 at 01:49
This still causes undefined behaviour if `other.mem` is uninitialized as in OP's scenario – M.M May 19 '19 at 03:59
@M.M: `other.mem` cannot be uninitialized, since that would mean that `myClass` is *broken*. The OP never said that they never initialized `mem` in the "Regular constructor, copy constructor" and such. Indeed, the presence of "Regular constructor" as a distinct construct from other constructors tells us that it is either a default constructor or a constructor that blocks the automatically generated default constructor. In short, the OP never claims that `other.mem` is uninitialized, and if it is, it is the fault of code that is not shown to us. – Nicol Bolas May 19 '19 at 04:08
@M.M: The crash comes from the fact that the OP is swapping an uninitialized value for an initialized one (the initial value of `other.mem`), then destroying the prvalue containing the uninitialized value. Indeed, the OP makes it clear with the sentence "you're swapping garbage **into** the `other` class". The "uninitialized" class the OP is thinking of is the one being constructed, not `other`. – Nicol Bolas May 19 '19 at 04:09
I interpreted the question differently but maybe you're right. Not sure what "uninitialized move" could possibly refer to in the title besides moving from an uninitialized object. (The object being constructed is not initialized yet, by definition). They talk about "detect" as well. Maybe they are mixing up move-construction with move-assignment as you say. – M.M May 19 '19 at 04:31

How to write move constructor to handle uninitialized move?

2 Answers2