From 14e2c0f8d70076e18bfccf5ac65317054faf1cf0 Mon Sep 17 00:00:00 2001 From: Nicholas Ormrod Date: Thu, 3 Apr 2014 20:02:55 -0700 Subject: [PATCH] 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 --- folly/FBVector.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/folly/FBVector.h b/folly/FBVector.h index a85f7570..a873b11b 100644 --- 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); } } -- 2.34.1