My colleague Mark Palange discovered a Boost.Coroutines scenario that can lead to stack corruption. The normal usage for a boost::coroutines::coroutine object is to repeatedly invoke its operator() method. Each entry to this method rebinds the coroutine_impl::m_result pointer to the address of a stack result_slot_type* variable. So far, so good. When a coroutine waits for a boost::coroutines::future object, the original operator() call exits, destroying the stack result_slot_type* variable. When that coroutine is later resumed by calling the future object's callback, coroutine_impl::m_result is *not* rebound: it's left set to the now-destroyed (possibly reused) address. Finally, when the coroutine exits, coroutine_impl::bind_result() blithely assigns through *m_result, thereby overwriting that slot on the stack. Now we're in Undefined Behavior territory. The attached patch file addresses the problem in two ways: 1. Each of the three methods that calls coroutine_impl::bind_result_pointer() (coroutine::call_impl(), coroutine::call_impl_nothrow(), coroutine_impl::run()) now uses an instance of a new helper class coroutine_impl::local_result_slot_ptr. This class contains the result_slot_type* variable and calls bind_result_pointer(&member) when constructed, wrapping the existing logic. But it also calls bind_result_pointer(NULL) when destroyed. This way, an invalid attempt to dereference *m_result is more likely to self-identify than to lead to mysterious, not obviously related side effects. 2. coroutine_impl::bind_result() now avoids the assignment if m_result is NULL. Possibly there are better behaviors for bind_result() on NULL m_result than silently bypassing the assignment, but this works for us, since our typical coroutine is void-valued anyway. If you're using boost::coroutines::coroutine and waiting on a boost::coroutines::future, please apply the attached patch to your copy of the Boost.Coroutines library. Alternatively, update the whole library with the new tarball: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=boost-coroutine-2009-12-01.tar.gz&directory=Concurrent%20Programming& diff -rc boost-coroutine/boost/coroutine/coroutine.hpp /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/coroutine.hpp *** boost-coroutine/boost/coroutine/coroutine.hpp Wed Apr 29 14:41:05 2009 --- /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/coroutine.hpp Tue Dec 1 15:25:52 2009 *************** *** 258,265 **** result_type call_impl(arg_slot_type args) { BOOST_ASSERT(m_pimpl); m_pimpl->bind_args(&args); ! result_slot_type * ptr; ! m_pimpl->bind_result_pointer(&ptr); m_pimpl->invoke(); return detail::fix_result(*m_pimpl->result()); --- 261,267 ---- result_type call_impl(arg_slot_type args) { BOOST_ASSERT(m_pimpl); m_pimpl->bind_args(&args); ! BOOST_DEDUCED_TYPENAME impl_type::local_result_slot_ptr ptr(m_pimpl); m_pimpl->invoke(); return detail::fix_result(*m_pimpl->result()); *************** *** 270,277 **** call_impl_nothrow(arg_slot_type args) { BOOST_ASSERT(m_pimpl); m_pimpl->bind_args(&args); ! result_slot_type * ptr; ! m_pimpl->bind_result_pointer(&ptr); if(!m_pimpl->wake_up()) return detail::optional_result(); --- 272,278 ---- call_impl_nothrow(arg_slot_type args) { BOOST_ASSERT(m_pimpl); m_pimpl->bind_args(&args); ! BOOST_DEDUCED_TYPENAME impl_type::local_result_slot_ptr ptr(m_pimpl); if(!m_pimpl->wake_up()) return detail::optional_result(); diff -rc boost-coroutine/boost/coroutine/detail/coroutine_impl.hpp /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/detail/coroutine_impl.hpp *** boost-coroutine/boost/coroutine/detail/coroutine_impl.hpp Sun Aug 20 13:11:09 2006 --- /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/detail/coroutine_impl.hpp Tue Dec 1 16:04:48 2009 *************** *** 89,95 **** } void bind_result(result_slot_type* res) { ! *m_result = res; } // Another level of indirecition is needed to handle --- 89,98 ---- } void bind_result(result_slot_type* res) { ! // This used to be unconditional. But m_result isn't always valid. ! if (m_result) { ! *m_result = res; ! } } // Another level of indirecition is needed to handle *************** *** 102,120 **** return m_result; } // This function must be called only for void // coroutines. It wakes up the coroutine. // Entering the wait state does not cause this // method to throw. void run() { arg_slot_type void_args; - result_slot_type * ptr = 0; // This dummy binding is required because // do_call expect args() and result() // to return a non NULL result. bind_args(&void_args); ! bind_result_pointer(&ptr); this->wake_up(); } protected: --- 105,157 ---- return m_result; } + /// This helper class packages data/logic originally found inline in + /// coroutine::call_impl() and call_impl_nothrow(), also + /// coroutine_impl::run(). + class local_result_slot_ptr + { + public: + local_result_slot_ptr(pointer pimpl): + m_pimpl(pimpl), + m_ptr(NULL) + { + m_pimpl->bind_result_pointer(&m_ptr); + } + ~local_result_slot_ptr() + { + // In the original use case, a coroutine could only be resumed by + // calling coroutine::operator() again, which would rebind the + // result pointer to a new valid value. But with the introduction + // of futures, it's possible to suspend a coroutine by waiting on + // a future object -- thus destroying the local result_slot_type* + // -- then resume that coroutine by calling the future's callback, + // bypassing coroutine::operator(). This used to leave an old, + // invalid result pointer in effect. Subsequent coroutine exit + // wrote through that pointer, munging a word of stack. Now we + // make a point of setting the bound result pointer NULL when the + // result_slot_type* to which it pointed vanishes, so that any + // attempt to dereference it will at least self-identify -- + // instead of producing arbitrary undefined behavior. + m_pimpl->bind_result_pointer(NULL); + } + + private: + pointer m_pimpl; + result_slot_type* m_ptr; + }; + // This function must be called only for void // coroutines. It wakes up the coroutine. // Entering the wait state does not cause this // method to throw. void run() { arg_slot_type void_args; // This dummy binding is required because // do_call expect args() and result() // to return a non NULL result. bind_args(&void_args); ! local_result_slot_pointer(this); this->wake_up(); } protected: diff -rc boost-coroutine/boost/coroutine/detail/self.hpp /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/detail/self.hpp *** boost-coroutine/boost/coroutine/detail/self.hpp Sun Aug 20 13:11:09 2006 --- /Users/nat/linden/viewer-20-topush/libraries/include/boost/coroutine/detail/self.hpp Wed Aug 5 16:50:17 2009 *************** *** 217,222 **** --- 217,235 ---- BOOST_ASSERT(m_pimpl); return m_pimpl->pending(); } + + /// @c true only if this @c self object was created by the passed @a coroutine + template <typename SomeCoroutine> + bool is_from(const SomeCoroutine& coroutine) const + { + // get_impl() only accepts non-const ref... a mistake, IMO. + return static_cast(coroutine_accessor::get_impl(const_cast(coroutine)).get()) == + static_cast(m_pimpl); + } + + /// opaque token used to correlate this 'self' with its corresponding coroutine + void* get_id() const { return m_pimpl; } + private: coroutine_self(impl_type * pimpl, detail::init_from_impl_tag) : m_pimpl(pimpl) {}