diff mbox

[FFmpeg-devel,1/5] avformat/mxfdec: fix klv_decode_ber_length return value usage

Message ID 20180527192154.25996-1-cus@passwd.hu
State Accepted
Commit f932e49aab09ffb150aab45746ac7ee669b22b14
Headers show

Commit Message

Marton Balint May 27, 2018, 7:21 p.m. UTC 
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Tomas Härdin May 27, 2018, 9:20 p.m. UTC | #1 
sön 2018-05-27 klockan 21:21 +0200 skrev Marton Balint:
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 7a42555562..40c9e0c3a9 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -372,6 +372,8 @@ static int64_t klv_decode_ber_length(AVIOContext *pb)
>          while (bytes_num--)
>              size = size << 8 | avio_r8(pb);
>      }
> +    if (size > INT64_MAX)
> +        return AVERROR_INVALIDDATA;
>      return size;
>  }
>  
> @@ -390,13 +392,17 @@ static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size)
>  
>  static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
>  {
> +    int64_t length;
>      if (!mxf_read_sync(pb, mxf_klv_key, 4))
>          return AVERROR_INVALIDDATA;
>      klv->offset = avio_tell(pb) - 4;
>      memcpy(klv->key, mxf_klv_key, 4);
>      avio_read(pb, klv->key + 4, 12);
> -    klv->length = klv_decode_ber_length(pb);
> -    return klv->length == -1 ? -1 : 0;
> +    length = klv_decode_ber_length(pb);
> +    if (length < 0)
> +        return length;
> +    klv->length = length;
> +    return 0;
>  }

This feels like the kind of thing that should have been caught ages
ago. Are there any other -1's like this hiding in mxfdec? I took a
quick look but didn't find much

/Tomas
Marton Balint May 27, 2018, 10:10 p.m. UTC | #2 
On Sun, 27 May 2018, Tomas Härdin wrote:

> sön 2018-05-27 klockan 21:21 +0200 skrev Marton Balint:
>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfdec.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 7a42555562..40c9e0c3a9 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -372,6 +372,8 @@ static int64_t klv_decode_ber_length(AVIOContext *pb)
>>          while (bytes_num--)
>>              size = size << 8 | avio_r8(pb);
>>      }
>> +    if (size > INT64_MAX)
>> +        return AVERROR_INVALIDDATA;
>>      return size;
>>  }
>>  
>> @@ -390,13 +392,17 @@ static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size)
>>  
>>  static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
>>  {
>> +    int64_t length;
>>      if (!mxf_read_sync(pb, mxf_klv_key, 4))
>>          return AVERROR_INVALIDDATA;
>>      klv->offset = avio_tell(pb) - 4;
>>      memcpy(klv->key, mxf_klv_key, 4);
>>      avio_read(pb, klv->key + 4, 12);
>> -    klv->length = klv_decode_ber_length(pb);
>> -    return klv->length == -1 ? -1 : 0;
>> +    length = klv_decode_ber_length(pb);
>> +    if (length < 0)
>> +        return length;
>> +    klv->length = length;
>> +    return 0;
>>  }
>
> This feels like the kind of thing that should have been caught ages
> ago. Are there any other -1's like this hiding in mxfdec? I took a
> quick look but didn't find much

The code was correct before bcd5d979aa377a16d7fa6b9993e269003281af5c 
because klv_decode_ber_length returned explict -1 back then.

Let's just hope no other place requires -1 error code, I can't seem to 
find similar cases either.

Thanks,
Marton
diff mbox

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 7a42555562..40c9e0c3a9 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -372,6 +372,8 @@  static int64_t klv_decode_ber_length(AVIOContext *pb)
         while (bytes_num--)
             size = size << 8 | avio_r8(pb);
     }
+    if (size > INT64_MAX)
+        return AVERROR_INVALIDDATA;
     return size;
 }
 
@@ -390,13 +392,17 @@  static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size)
 
 static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
 {
+    int64_t length;
     if (!mxf_read_sync(pb, mxf_klv_key, 4))
         return AVERROR_INVALIDDATA;
     klv->offset = avio_tell(pb) - 4;
     memcpy(klv->key, mxf_klv_key, 4);
     avio_read(pb, klv->key + 4, 12);
-    klv->length = klv_decode_ber_length(pb);
-    return klv->length == -1 ? -1 : 0;
+    length = klv_decode_ber_length(pb);
+    if (length < 0)
+        return length;
+    klv->length = length;
+    return 0;
 }
 
 static int mxf_get_stream_index(AVFormatContext *s, KLVPacket *klv, int body_sid)
@@ -486,7 +492,10 @@  static int mxf_decrypt_triplet(AVFormatContext *s, AVPacket *pkt, KLVPacket *klv
         av_aes_init(mxf->aesc, s->key, 128, 1);
     }
     // crypto context
-    avio_skip(pb, klv_decode_ber_length(pb));
+    size = klv_decode_ber_length(pb);
+    if (size < 0)
+        return size;
+    avio_skip(pb, size);
     // plaintext offset
     klv_decode_ber_length(pb);
     plaintext_size = avio_rb64(pb);