Remote code execution vulnerability in VLC remains unpatched

The bug itself: https://trac.videolan.org/vlc/ticket/22474

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?