[FFmpeg-devel,5/5] avformat/mxfdec: fix and enhance RIP KLV length checks

Submitted by Marton Balint on April 11, 2019, 11:09 p.m.

Details

Message ID 20190411230920.6630-5-cus@passwd.hu
State Accepted
Commit 3dee6c09970dd0defad91e8270f66a4be3b6570d
Headers show

Commit Message

Marton Balint April 11, 2019, 11:09 p.m.
KLV length is BER encoded (variable size), but the code assumed the encoding to
always use 4 bytes.

Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Tomas Härdin April 14, 2019, 4 p.m.
fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint:
> KLV length is BER encoded (variable size), but the code assumed the encoding to
> always use 4 bytes.
> 
> Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf.
> 
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 6f0f87763d..a69f2f1996 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s)
>          goto end;
>      avio_seek(s->pb, file_size - length, SEEK_SET);
>      if (klv_read_packet(&klv, s->pb) < 0 ||
> -        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) ||
> -        klv.length != length - 20)
> +        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key))
>          goto end;
> +    if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) {
> +        av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n");
> +        goto end;
> +    }

Looks OK.

Aside: Looking at klv_read_packet(), I'm suspicious of its use of
mxf_read_sync(). That feels like something that only belongs in
mxf_read_header(), to deal with run-in. Baptiste added it in
cabe2527ef3. FATE passes without it.

/Tomas
Marton Balint April 28, 2019, 8:17 p.m.
On Sun, 14 Apr 2019, Tomas Härdin wrote:

> fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint:
>> KLV length is BER encoded (variable size), but the code assumed the encoding to
>> always use 4 bytes.
>> 
>> Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf.
>> 
>> > Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfdec.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 6f0f87763d..a69f2f1996 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s)
>>          goto end;
>>      avio_seek(s->pb, file_size - length, SEEK_SET);
>>      if (klv_read_packet(&klv, s->pb) < 0 ||
>> -        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) ||
>> -        klv.length != length - 20)
>> +        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key))
>>          goto end;
>> +    if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) {
>> +        av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n");
>> +        goto end;
>> +    }
>
> Looks OK.

Thanks, applied.

>
> Aside: Looking at klv_read_packet(), I'm suspicious of its use of
> mxf_read_sync(). That feels like something that only belongs in
> mxf_read_header(), to deal with run-in. Baptiste added it in
> cabe2527ef3. FATE passes without it.

I wonder if it is there to allow some byte based seeking or handle damaged 
files more gracefully? I agree it is strange a bit, maybe Baptise can chip 
in...

Regards,
Marton
Tomas Härdin April 29, 2019, 7:38 a.m.
sön 2019-04-28 klockan 22:17 +0200 skrev Marton Balint:
> 
> On Sun, 14 Apr 2019, Tomas Härdin wrote:
> 
> > fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint:
> > > KLV length is BER encoded (variable size), but the code assumed the encoding to
> > > always use 4 bytes.
> > > 
> > > Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf.
> > > 
> > > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > 
> > > ---
> > >  libavformat/mxfdec.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 6f0f87763d..a69f2f1996 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s)
> > >          goto end;
> > >      avio_seek(s->pb, file_size - length, SEEK_SET);
> > >      if (klv_read_packet(&klv, s->pb) < 0 ||
> > > -        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) ||
> > > -        klv.length != length - 20)
> > > +        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key))
> > >          goto end;
> > > +    if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) {
> > > +        av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n");
> > > +        goto end;
> > > +    }
> > 
> > Looks OK.
> 
> Thanks, applied.
> 
> > 
> > Aside: Looking at klv_read_packet(), I'm suspicious of its use of
> > mxf_read_sync(). That feels like something that only belongs in
> > mxf_read_header(), to deal with run-in. Baptiste added it in
> > cabe2527ef3. FATE passes without it.
> 
> I wonder if it is there to allow some byte based seeking or handle damaged 
> files more gracefully? I agree it is strange a bit, maybe Baptise can chip 
> in...

I *suspect* it has something to do with seeking in frame wrapped files
with crappy or non-existing indexes. But then it would be best if it
were used only in mxf_read_seek()

/Tomas

Patch hide | download patch | download mbox

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 6f0f87763d..a69f2f1996 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -3126,9 +3126,12 @@  static void mxf_read_random_index_pack(AVFormatContext *s)
         goto end;
     avio_seek(s->pb, file_size - length, SEEK_SET);
     if (klv_read_packet(&klv, s->pb) < 0 ||
-        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) ||
-        klv.length != length - 20)
+        !IS_KLV_KEY(klv.key, mxf_random_index_pack_key))
         goto end;
+    if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) {
+        av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n");
+        goto end;
+    }
 
     avio_skip(s->pb, klv.length - 12);
     mxf->footer_partition = avio_rb64(s->pb);