The response seems … snippy:
> If you land on this ticket through a news article claiming a critical flaw in VLC, I suggest you to read the above comment first and reconsider your (fake) news sources.
So, the code at hand is as follows:
for (std::vector<matroska_stream_c*>::reverse_iterator i = streams.rbegin();
i != streams.rend(); ++i)
{
matroska_stream_c *p_s = *i;
if( !p_s->isUsed() )
{
std::advance(i, 1);
streams.erase( i.base() );
delete p_s;
}
}
My big assumption here is that we're going to want to remove the p_s that we delete; I think that's a safe bet given the function's name.The key lines, I think, are the advance and erase. They … do not make sense to me. The erase call, as a side effect, invalidates the iterator. Regardless, I would expect us to be deleting the original position of i from the vector, not the advanced position. That is, I'd expect something like,
auto old_i = i;
++i;
streams.erase(old_i.base());
In addition to an, else { ++i }
and the removal of the ++i from the for loop's last part. You need to step the iterator prior to erasing, then erase the old position, and then not inadvertently step the iterator a second time in the for() call.Alternatively, call remove_if() followed by a range-erase; though here I think you'd need to supply remove_if with a function with side-effects (for the delete), which is a bit "ew" in the understandability/readability department. remove_if would probably be the way to go, and a lot clearer, if the vector were holding something that RAII'd/understood the ownership of its contents, instead of a raw pointer. (Edit: actually, now that I've thought about it, no, even with the side-effects in the predicate, it's worth it to not need to manually deal with iterators.)
I haven't tried to reproduce it, but no crashes does not mean there isn't a bug, and the code looks wrong.
Am I missing something?