Steve Lorimer Steve Lorimer - 18 days ago 6
C++ Question

C++17 expression evaluation order and std::move

Whilst refactoring some code today to change raw pointers to

std::unique_ptr
, I encountered a segmentation fault due to an order of evaluation bug.

The old code did something like the following:

void add(const std::string& name, Foo* f)
{
_foo_map[name] = f;
}

void process(Foo* f)
{
add(f->name, f);
}


The first, naive, refactoring of the code to use
std::unique_ptr
:

void add(const std::string& name, std::unique_ptr<Foo> f)
{
_foo_map[name] = std::move(f);
}

void process(std::unique_ptr<Foo> f)
{
add(f->name, std::move(f)); // segmentation-fault on f->name
}


The refactored code causes a segmentation fault, because the 2nd argument (
std::move(f)
) is processed first, and then the 1st argument (
f->name
) dereferences a moved-from variable, boom!

Possible fixes to this are to obtain a handle on
Foo::name
before moving it in the call to
add
:

void process(std::unique_ptr<Foo> f)
{
const std::string& name = f->name;
add(name, std::move(f));
}


Or perhaps:

void process(std::unique_ptr<Foo> f)
{
Foo* fp = f.get();
add(fp->name, std::move(f));
}


Both of these solutions require extra lines of code, and don't seem nearly as composable as the original (albeit UB) call to
add
.

Questions:


Answer

For me these 2 proposals look bad. In either of those you are giving your Foo object away with the move. That means you can no longer make any assumptions about it's state after that. It could be deallocated inside the add function before the first argument (reference to the string or pointer to the object) is processed. Yes, it would work in the current implementation but it could break as soon as anyone touches the implementation of add or anything deeper in it.

The safe ways:

  • Make a copy of the string and pass that as the first argument to add
  • Refactor your add method that it only takes a single argument of type Foo and extracts Foo::name inside the method and doesn't take it as an argument. However you still have the same issue inside the add method.

In the second approach you should be able get around the evaluation order problem by first creating a new map entry (with default value) and getting a mutable reference to it and assigning the value afterwards:

auto& entry = _foo_map[f->name];
entry = std::move(f);

Not sure whether your map implementation supports getting a mutable reference to entries, but for many it should work.

If I think about it again you might also go for the "copy the name" approach. It needs to be copied anyway for the map key. If you copy it manually you can move it for the key there's no overhead.

std::string name = f->name;
_foo_map[std::move(name)] = std::move(f);

Edit: As pointed out in the comments it should be possible to directly assign _foo_map[f->name] = std::move(f) inside the add function since the evaluation order is guaranteed here.

Comments