kwiver
kwiver copied to clipboard
Undefined behavior when dereferencing iterator for kwiver::arrows::ocv::descriptor_set after copying
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.