Remove disallowed &* of FwdIterator
authorNicholas Ormrod <njormrod@fb.com>
Mon, 24 Feb 2014 18:40:39 +0000 (10:40 -0800)
committerDave Watson <davejwatson@fb.com>
Fri, 28 Feb 2014 21:59:48 +0000 (13:59 -0800)
commit764261c12ab059cd15607a1c2d2a7ff015dfe7ce
tree9d0d5888b84711154677259c463bc1e88f6fa805
parentd364aea0dec7121fc2185f65bc89a08774ec3a65
Remove disallowed &* of FwdIterator

Summary:
Iterators are not required to dereference into lvalues, so
taking the address of the dereferenced value of a general iterator
may cause a compile-time error.

This bug was observed when compiling clang-3.4. Clang uses a custom
iterator type when calling fbstring::replace, whose dereference operator
returns a char (instead of the 'expected' const char&).

FBString takes the address of the dereference in order to test if the
iterator is actually an iterator referencing its own data. This protects
the string from data trampling in certain cases. See the added test case
for an example.

For sequence containers, the standard specifies that supplying interal
iterators for such operations is forbidden. The standard also states
that the iterators passed into containers will be dereferenced at each
location exactly once. The standard (from by too-brief inspection) does
not specify either of these restrictions for strings, which I find odd.

As a compromise between safety and strict compliance, the offending code
is now only run when the iterator type is either fbstring::iterator or
fbstring::const_iterator. In these cases, we know that it is safe to
both dereference the iterator multiple times and to take its
dereference's address.

While fixing this error, I noticed that fbstring::replaceImpl was
public. It is now private.

Test Plan:
Added a new test case to FBStringTest.cpp.

fbconfig -r folly && fbmake opt && fbmake runtests_opt

Reviewed By: delong.j@fb.com

FB internal diff: D1185655
folly/FBString.h
folly/test/FBStringTest.cpp