Range Adapters (map_keys, map_values) cause undefined behavior (segv, etc) when applied to R-Values, especially in the context of BOOST_FOREACH
Hi! I've filed a ticket (https://svn.boost.org/trac/boost/ticket/9578) for this issue two weeks ago, but I see no action there. Does anyone else think that this (see subj) is a problem? To spare the trouble of following the link, here is the essence of the problem, as I see it. (The ticket has a simple test case attached.) 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). Any comments are welcome! Thanks, - Igor
Hi Igor, This is a known issue: see https://svn.boost.org/trac/boost/ticket/7630. (That bug talks about C++11 range-based for loops, but the issue with BOOST_FOREACH is the same). I'm not sure what the status of this bug is. Jeffrey Yasskin posted a patch to that bug which I reviewed over email, but I haven't heard from him about it since. Note that if you use C++11, a workaround is to use boost::for_each with a lambda instead, so BOOST_FOREACH(SomeType v, range_rvalue | adaptor) { statements } turns into boost::for_each(range_rvalue | adaptor, [&](SomeType v) { statements }); Since the temporary lives until the end of the statement, and the whole loop is contained within the statement, there is no more undefined behaviour. Regards, Nate
On Thu, Jan 30, 2014 at 12:41 PM, Nathan Ridge <zeratul976@hotmail.com> wrote:
Hi Igor,
This is a known issue: see https://svn.boost.org/trac/boost/ticket/7630. (That bug talks about C++11 range-based for loops, but the issue with BOOST_FOREACH is the same).
I'm not sure what the status of this bug is. Jeffrey Yasskin posted a patch to that bug which I reviewed over email, but I haven't heard from him about it since.
Why was the review not done on the bug? I'm tracking that bug and would like to see it resolved, and it seems the patch is just fine. Are there any issues still that need to be addressed elsewhere? What can I do to help expedite this process?
Note that if you use C++11, a workaround is to use boost::for_each with a lambda instead, so
BOOST_FOREACH(SomeType v, range_rvalue | adaptor) { statements }
turns into
boost::for_each(range_rvalue | adaptor, [&](SomeType v) { statements });
Since the temporary lives until the end of the statement, and the whole loop is contained within the statement, there is no more undefined behaviour.
This is not an acceptable solution to the original problem.
Regards, Nate
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
From: dberris@google.com
On Thu, Jan 30, 2014 at 12:41 PM, Nathan Ridge <zeratul976@hotmail.com> wrote:
This is a known issue: see https://svn.boost.org/trac/boost/ticket/7630. (That bug talks about C++11 range-based for loops, but the issue with BOOST_FOREACH is the same).
I'm not sure what the status of this bug is. Jeffrey Yasskin posted a patch to that bug which I reviewed over email, but I haven't heard from him about it since.
Why was the review not done on the bug?
Jeffrey asked me to review it over email and I responded in kind. I expected that he would post a follow-up patch that addresses the points made in the review shortly thereafter. I posted my review comments in the bug now.
I'm tracking that bug and would like to see it resolved, and it seems the patch is just fine. Are there any issues still that need to be addressed elsewhere? What can I do to help expedite this process?
I think that the approach is fine. The suggestions I make in my review are fairly minor (nonetheless I would like to see them addressed). We should also make sure Neil is on board with this approach. Since this is a significant change, I would like to have his approval before committing anything. (Now that we use Git should I be saying "before pushing anything"? (-: ). Finally, once we know the approach has consensus, someone needs to replicate the patch for all the adaptors (Jeffrey's patch is for 'filtered' only). Regards, Nate
participants (3)
-
Dean Michael Berris -
Igor Lubashev -
Nathan Ridge