kwiver icon indicating copy to clipboard operation
kwiver copied to clipboard

Undefined behavior when dereferencing iterator for kwiver::arrows::ocv::descriptor_set after copying

Open predicative opened this issue 4 years ago • 0 comments

When running stabilize_images.pipe, I reliably get a segfault on the second input frame. I investigated and narrowed the problem down to here:

https://github.com/Kitware/kwiver/blob/97c76e182eb991d41f6d6c9fd60ad046d26910aa/arrows/core/track_features_core.cxx#L376-L378

Changing the last line to auto dit = curr_desc->begin(); removes the issue. This appears to be because the (unelidable) copy in the existing code puts the iterator in a bad state because the std::function returned by kwiver::arrows::ocv::descriptor_set::get_iter_next_func returns an internal reference (see here), which is stored in the m_cur_val_ptr of the iterator but invalidated by the destruction of the original. The first subsequent use of *dit returns a bogus value (the undefined behavior), ultimately crashing somewhere completely different on the next frame when trying to interpret said value to perform a dynamic cast:

https://github.com/Kitware/kwiver/blob/97c76e182eb991d41f6d6c9fd60ad046d26910aa/arrows/ocv/descriptor_set.cxx#L90

This was a debug build of recent master (2fca7dacd) with GCC 9.3.1. That said, by my reading of https://en.cppreference.com/w/cpp/utility/functional/function/function this is expected behavior for std::function:

3-4) [The copy constructor c]opies [...] the target of other to the target of *this

As I understand it, this is contrary to the documentation of kwiver::vital::iterator, which says that "the next-value function's state is not fully copied" (see here).

I suppose the fix would be to wrap m_next_value_func in a shared_ptr, which should give the documented shallow-copy semantics at the cost of another pointer indirection. @Purg, thoughts? If that sounds like the right way forward I can work on a PR.

predicative avatar May 18 '20 13:05 predicative