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?

Enchondroma

Do you need a memory buffer, or do you really need std::string? – Flourishing

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? – Enchondroma

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;) – Moulmein

@Moulmein 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). – Enchondroma

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. – Flourishing

@Moulmein This doesn't completely solve the problem. free(mem) is still potentially freeing a garbage pointer. – Enchondroma

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. – Consensus

Related: What is The Rule of Three? and What is the copy-and-swap idiom? – Kippar

@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 – Enchondroma

@Mahkoe Calling a constructor on initialized memory is undefined behavior. You are accessing the value of an uninitialized pointer (mem). – Kippar

@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. – Consensus

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). – Shawanda

"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." – Moulmein

Answer 1

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.
};

Coburg

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... – Verdaverdant

@Verdaverdant 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… – Coburg

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. – Haywoodhayyim

Answer 2

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))
{}

Haywoodhayyim

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. – Coburg

@MichaelKenzel: Not if you need your constructor to be constexpr. C++20 fixes that issue, though. – Haywoodhayyim

This still causes undefined behaviour if other.mem is uninitialized as in OP's scenario – Shawanda

@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. – Haywoodhayyim

@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. – Haywoodhayyim

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. – Shawanda