Bugfix in iterator emplace

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

diff --git a/folly/FBVector.h b/folly/FBVector.h
index a85f7570e55ade8f0f17b95fe17ac3b762bdecd1..a873b11b295778ba4c7f000de5255ddcea562e61 100644 (file)
--- a/folly/FBVector.h
+++ b/folly/FBVector.h
@@ -1412,9 +1412,15 @@ private: // we have the private section first because it defines some macros
         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);
       }
     }