[vlc-devel] [PATCH 6/7] demux:mkv: don't read further than our parent or its parent

[Steve Lhomme]

On Sat, Nov 18, 2017 at 1:57 PM, Romain Vimont <rom at rom1v.com> wrote:
>
>
> Le lundi 13 novembre 2017 à 18:08 +0100, Steve Lhomme a écrit :
>> ---
>>  modules/demux/mkv/Ebml_parser.cpp | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/modules/demux/mkv/Ebml_parser.cpp b/modules/demux/mkv/Ebml_parser.cpp
>> index 330edd4c7c..5f663c6a45 100644
>> --- a/modules/demux/mkv/Ebml_parser.cpp
>> +++ b/modules/demux/mkv/Ebml_parser.cpp
>> @@ -158,6 +158,27 @@ EbmlElement *EbmlParser::Get( int n_call )
>>      if( p_prev )
>>          p_prev->SkipData( *m_es, EBML_CONTEXT(p_prev) );
>>
>> +    uint64_t i_max_read;
>> +    if (mi_level == 0)
>> +        i_max_read = UINT64_MAX;
>> +    else if (!m_el[mi_level-1]->IsFiniteSize())
>> +        i_max_read = UINT64_MAX;
>> +    else if (!p_prev)
>> +        i_max_read = m_el[mi_level-1]->GetSize();
>
> Here, i_max_read may be set to 0.

Technically yes. But that would mean there is no previous element. In
other words the Master element has no children. In the case of
Matroska it's not legal. But it should possible to doctor such a file
for test purposes.

>> +    else {
>> +        size_t size_lvl = mi_level;
>> +        while ( size_lvl && m_el[size_lvl-1]->IsFiniteSize() &&
>> +                m_el[size_lvl-1]->GetEndPosition() == m_el[size_lvl]->GetEndPosition() )
>> +            size_lvl--;
>> +        if (size_lvl == 0 || !m_el[size_lvl-1]->IsFiniteSize() )
>> +            i_max_read = UINT64_MAX;
>> +        else {
>> +            uint64 top = m_el[size_lvl-1]->GetEndPosition();
>> +            uint64 bom = m_el[mi_level]->GetEndPosition();
>> +            i_max_read = top - bom;
>> +        }
>> +    }
>> +
>>      // If the parent is a segment, use the segment context when creating children
>>      // (to prolong their lifetime), otherwise just continue as normal
>>      EbmlSemanticContext e_context =
>> @@ -167,7 +188,7 @@ EbmlElement *EbmlParser::Get( int n_call )
>>
>>      /* Ignore unknown level 0 or 1 elements */
>>      m_el[mi_level] = m_es->FindNextElement( e_context,
>> -                                            i_ulev, UINT64_MAX,
>> +                                            i_ulev, i_max_read,
>
> When i_max_read is 0, an assertion fails:
>
> vlc: src/EbmlElement.cpp:480: libebml::EbmlElement* libebml::EbmlElement::SkipData(libebml::EbmlStream&, const libebml::EbmlSemanticContext&, libebml::EbmlElement*, bool): Assertion `ElementPosition < SizePosition' failed.
> Aborted
>
> Here is a file to reproduce: https://tmp.rom1v.com/fail.mkv

OK, I will give it a try.

This patch also has an issue as i_max_read may overflow (ending up
with a large value). In reality it's still better than what was there
before and will continue working in that case. To really fix this we
need to rework the parser so it works kinda like EbmlMaster reads its
children or the next element in libebml. In other words knowing the
current boundaries allowed or when it should go over the boundaries.
This proposed patch tries to get close enough to this without a big
refactoring.

> Replacing i_max_read by UINT64_MAX avoids the problem (but this is not a
> fix).
>
>>                                              (  mb_dummy | (mi_level > 1) ), 1 );
>>      if( i_ulev > 0 )
>>      {
>> --
>> 2.14.2
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel