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

[Steve Lhomme]

BTW can you fill an issue on Trac an assign it to me ? So we don't
forget it for the 3.0 release. Thanks

On Sat, Nov 18, 2017 at 2:18 PM, Steve Lhomme <robux4 at gmail.com> wrote:
> 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