Adapters (map_keys, map_values) cause undefined behavior (segv, etc) when applied to R-Values, especially in the context of BOOST_FOREACH

[Igor Lubashev <ilubashe@…>]

When applied to r-values (for example, maps returned by value from a function), adapters do not make a copy of the r-value but just capture a reference to it. This makes the returned range_iterator dangerous, if it is used after the r-value (the temporary) is destroyed.

In particular, the following use causes undefined behavior on all compilers I'd tested on: g++ (4.1.2, 4.4.3, 4.6.3) and Visual Studio (15 [VC9], 16 [VC10]).

BOOST_FOREACH(const string &s, return_map_by_value() | map_keys)
  ...

Note that the following works fine (since BOOST_FOREACH takes care of differentiating between l-values and r-values and does the right thing to r-values):

BOOST_FOREACH(const MapType::value_type &v, return_map_by_value())
  ...

It seems that boost::range::adapters need to do a similar thing to what BOOST_FOREACH is does to identify r-values and make a copy (which the compiler is likely to optimize away).

Attached is a test sample and the resulting behavior. The output is from code compiled with g++ 4.6.3 with no optimizations.

$ g++ -I ../include/ container_test.cpp
$ ./a.out
Test #1 (local variable)
  Generating map
key = '12345'
key = 'QWERT'
key = 'igor!'
Test #2 (value_type)
  Generating map
key = '12345'
key = 'QWERT'
key = 'igor!'
Test #3 (temp ref)
  Generating map
key = '12345'
key = 'QWERT'
key = 'igor!'
Test #4 (range_iterator)
  Generating map
key = '12345'
key = 'QWERT'
key = 'igor!'
Test #5 (bug)
  Generating map
key = '12345APãr`ãr¨ârãr1 ãrÿÿÿÿABCDE1ãrÿÿÿÿQWERTAâr½™ÿÀâr ârHãr¸ãr1àárÿÿÿÿqwertAärH½™ÿPärärHãr¸ãrAårÐãârxârAÀãrÐãr¨ârãrAÀärX™ÿårÐärHãr¸ãrAäârxârA€ärär¨ârãrA€årؽ™ÿÐårårHãr¸ãrA°ârPåârxârA@årPår¨ârãr

[Igor Lubashev <ilubashe@…>]

Test to demonstrate undefined behavior of boost::range::adapter::map_keys when applied to an r-value and used with BOOST_FOREACH

[Neil Groves]

See: ​https://svn.boost.org/trac/boost/ticket/5475

BOOST_FOREACH is considered to be deprecated and in maintenance mode. There are no simple solutions for C++03 that don't cause unacceptable performance overhead and enormous complexity.

[ilubashe@…]

This bug has little to do with BOOST_FOREACH. This is a boost::range problems. You'll get the same problem with C++11 "for" loops.

for(const string &s : return_map_by_value() | map_keys)

[Neil Groves]

Yes you would, but I have no solution for this. It seems to me to be a lifetime issue that is not fixable without enormous detrimental impact on other code. I closed the ticket not due to unwillingness to alter the code, but due to having no good solution. If we alter the semantics to require copying this has huge impact on some ranges. If we automatically move this would either require large performance overhead due to bloated adapters and would alter the semantic to have other surprising features.

I'll see if you or anyone else have any suggestions for what you actually would like done. AFAICT it simply is a general C++ issue. You have to be careful with lifetimes. Of course if there is a solution I have overlooked that can make things better with little impact on other cases I would jump at it immediately.

[ilubashe@…]

I called out BOOST_FOREACH in filing the bug, because BOOST_FOREACH has the solution to a very similar problem it had to deal with.

I would certainly not propose range adapter to copy values wholesale. The only values that need copying are r-values. In fact, I would be very surprised if there is ever a case when you do NOT want to copy an r-value in a range adapter. The l-values should certainly never be copied.

BOOST_FOREACH has a way to tell which values are l-values and which are r-values.

There is also a related bug: #7630

[Neil Groves]

I'm marking this as a duplicate of 7630 now since that seems to be the more general issue. I've been worrying about this lifetime issue for sometime, but haven't a good solution. I spent a little time looking at the BOOST_FOREACH implementations and I'll take a look at some of the suggestions made for ticket 7630. It might be that this can now be made to work well with the C++11 support becoming much more mature and the rvalue support that much more reliable.

I hope I don't cause any offence by collapsing this into one defect. Please feel free to make comments on the #7630 ticket.

Since there is even an example solution I should be able to find a way to make this work. I'll obviously have to take care to ensure that the fix doesn't introduce any unexpected nasty surprises.

Thanks for helping.