Skip to content

Commit

Permalink
Bugfix in iterator emplace
Browse files Browse the repository at this point in the history
Summary:
Subtle bugfix to fbvector

Reproduction requirements:
- emplace into a vector at a non-end position
- the vector has enough capacity for the extra elements
- the value type's internal state is poisoned after a move

What should happen, explained by picture:

INITIAL SETUP

abc1234XY____
^
want to insert two elements here

FIRST STEP: uninitialized move-construction
x and y are initialized, but could be in an invalid state

abc1234xyXY__

SECOND STEP: move-assignment, in reverse order

abc123xy4XY__
abc12xy34XY__
abc1xy234XY__
abcxy1234XY__

What actually happens:
An indexing error causes the entire tail (##1234xy##) to be
move-assigned, instead of the intended head of the tail (##1234##).

If the data type's move operator does not invalidate its own data (i.e.
move is essentially a copy, as is the case with atomic types) then the
move assignment of ##y## onto ##Y## and ##x## onto ##X## is basically a
no-op. However, for types that do invalidate their own data when being
the source of a move (e.g. non-empty vectors and fbvectors), then the
end result will have bad data in place of ##X## and ##Y##.

Detection:
This bug has lain dormant for a while.

Very few people emplace into a vector, and those few who do could be
saved by using a copy-movable type like int.

The original test case did not use a data type which poisoned itself
after being the source of a move, so the tests did not notice any
oddities.

A new testsuite, which does poison itself, did notice.

Test Plan:
re-enable the test/stl_test mega-test. Run it. All tests
pass.

fbconfig -r folly && fbmake runtests_opt

Run fbvector through the new test suite (which initially caught the
error). It now passes.

Reviewed By: robbert@fb.com

FB internal diff: D1257258
  • Loading branch information
Gownta authored and ptarjan committed Apr 9, 2014
1 parent fbee7f6 commit 14e2c0f
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions folly/FBVector.h
Original file line number Diff line number Diff line change
Expand Up @@ -1412,9 +1412,15 @@ class fbvector : private boost::totally_ordered<fbvector<T, Allocator>> {
impl_.e_ += n;
} else {
D_uninitialized_move_a(impl_.e_, impl_.e_ - n, impl_.e_);
try {
std::copy_backward(std::make_move_iterator(position),
std::make_move_iterator(impl_.e_ - n), impl_.e_);
} catch (...) {
D_destroy_range_a(impl_.e_ - n, impl_.e_ + n);
impl_.e_ -= n;
throw;
}
impl_.e_ += n;
std::copy_backward(std::make_move_iterator(position),
std::make_move_iterator(impl_.e_ - n), impl_.e_);
D_destroy_range_a(position, position + n);
}
}
Expand Down

0 comments on commit 14e2c0f

Please sign in to comment.