Bugfix in iterator emplace
authorNicholas Ormrod <njormrod@fb.com>
Fri, 4 Apr 2014 03:02:55 +0000 (20:02 -0700)
committerptarjan <ptarjan@fb.com>
Wed, 9 Apr 2014 03:58:39 +0000 (20:58 -0700)
commit14e2c0f8d70076e18bfccf5ac65317054faf1cf0
tree8645ad795059300b89e84ff0f54539478bee434c
parentfbee7f6b6b7c090a3a906c4351dde9165f03190f
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