[FFmpeg-devel,01/12] avformat/mxfdec: store next_klv in KLVPacket

Submitted by Marton Balint on June 10, 2018, 10:36 a.m.

Details

Message ID 20180610103650.10155-1-cus@passwd.hu
State Accepted
Commit bfa0b50441785871062b507ece63bfaf63f67dfd
Headers show

Commit Message

Marton Balint June 10, 2018, 10:36 a.m.
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxf.h    |  1 +
 libavformat/mxfdec.c | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Tomas Härdin June 13, 2018, 2:39 p.m.
sön 2018-06-10 klockan 12:36 +0200 skrev Marton Balint:
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxf.h    |  1 +
>  libavformat/mxfdec.c | 13 ++++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/mxf.h b/libavformat/mxf.h
> index 19f8d8a9f5..93bc2cd075 100644
> --- a/libavformat/mxf.h
> +++ b/libavformat/mxf.h
> @@ -62,6 +62,7 @@ typedef struct KLVPacket {
>      UID key;
>      int64_t offset;
>      uint64_t length;
> +    int64_t next_klv;
>  } KLVPacket;
>  
>  typedef struct MXFCodecUL {
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index b3d3e237c0..a5c5fb3b8a 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -392,7 +392,7 @@ 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;
> +    int64_t length, pos;
>      if (!mxf_read_sync(pb, mxf_klv_key, 4))
>          return AVERROR_INVALIDDATA;
>      klv->offset = avio_tell(pb) - 4;
> @@ -402,6 +402,10 @@ static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
>      if (length < 0)
>          return length;
>      klv->length = length;
> +    pos = avio_tell(pb);
> +    if (pos > INT64_MAX - length)
> +        return AVERROR_INVALIDDATA;

I wonder, can pos be negative? That is, can avio_tell() fail? Else it
looks OK

/Tomas
Marton Balint June 13, 2018, 6:27 p.m.
On Wed, 13 Jun 2018, Tomas Härdin wrote:

> sön 2018-06-10 klockan 12:36 +0200 skrev Marton Balint:
>> > Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxf.h    |  1 +
>>  libavformat/mxfdec.c | 13 ++++++++-----
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/mxf.h b/libavformat/mxf.h
>> index 19f8d8a9f5..93bc2cd075 100644
>> --- a/libavformat/mxf.h
>> +++ b/libavformat/mxf.h
>> @@ -62,6 +62,7 @@ typedef struct KLVPacket {
>>      UID key;
>>      int64_t offset;
>>      uint64_t length;
>> +    int64_t next_klv;
>>  } KLVPacket;
>>  
>>  typedef struct MXFCodecUL {
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index b3d3e237c0..a5c5fb3b8a 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -392,7 +392,7 @@ 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;
>> +    int64_t length, pos;
>>      if (!mxf_read_sync(pb, mxf_klv_key, 4))
>>          return AVERROR_INVALIDDATA;
>>      klv->offset = avio_tell(pb) - 4;
>> @@ -402,6 +402,10 @@ static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
>>      if (length < 0)
>>          return length;
>>      klv->length = length;
>> +    pos = avio_tell(pb);
>> +    if (pos > INT64_MAX - length)
>> +        return AVERROR_INVALIDDATA;
>
> I wonder, can pos be negative? That is, can avio_tell() fail? Else it
> looks OK

Although it is not documented behaviour, but it can't be negative in the 
current implementation (if pb is not NULL). I can add a check if that is 
preferred.

Regards,
Marton
Tomas Härdin June 13, 2018, 7:02 p.m.
ons 2018-06-13 klockan 20:27 +0200 skrev Marton Balint:
> 
> On Wed, 13 Jun 2018, Tomas Härdin wrote:
> 
> > sön 2018-06-10 klockan 12:36 +0200 skrev Marton Balint:
> > > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > 
> > > ---
> > >  libavformat/mxf.h    |  1 +
> > >  libavformat/mxfdec.c | 13 ++++++++-----
> > >  2 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libavformat/mxf.h b/libavformat/mxf.h
> > > index 19f8d8a9f5..93bc2cd075 100644
> > > --- a/libavformat/mxf.h
> > > +++ b/libavformat/mxf.h
> > > @@ -62,6 +62,7 @@ typedef struct KLVPacket {
> > >      UID key;
> > >      int64_t offset;
> > >      uint64_t length;
> > > +    int64_t next_klv;
> > >  } KLVPacket;
> > >  
> > >  typedef struct MXFCodecUL {
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index b3d3e237c0..a5c5fb3b8a 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -392,7 +392,7 @@ 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;
> > > +    int64_t length, pos;
> > >      if (!mxf_read_sync(pb, mxf_klv_key, 4))
> > >          return AVERROR_INVALIDDATA;
> > >      klv->offset = avio_tell(pb) - 4;
> > > @@ -402,6 +402,10 @@ static int klv_read_packet(KLVPacket *klv,
> > > AVIOContext *pb)
> > >      if (length < 0)
> > >          return length;
> > >      klv->length = length;
> > > +    pos = avio_tell(pb);
> > > +    if (pos > INT64_MAX - length)
> > > +        return AVERROR_INVALIDDATA;
> > 
> > I wonder, can pos be negative? That is, can avio_tell() fail? Else
> > it
> > looks OK
> 
> Although it is not documented behaviour, but it can't be negative in
> the 
> current implementation (if pb is not NULL). I can add a check if that
> is 
> preferred.

I'm pretty sure there's a check for pb==NULL further up in lavf already

/Tomas

Patch hide | download patch | download mbox

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index 19f8d8a9f5..93bc2cd075 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -62,6 +62,7 @@  typedef struct KLVPacket {
     UID key;
     int64_t offset;
     uint64_t length;
+    int64_t next_klv;
 } KLVPacket;
 
 typedef struct MXFCodecUL {
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index b3d3e237c0..a5c5fb3b8a 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -392,7 +392,7 @@  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;
+    int64_t length, pos;
     if (!mxf_read_sync(pb, mxf_klv_key, 4))
         return AVERROR_INVALIDDATA;
     klv->offset = avio_tell(pb) - 4;
@@ -402,6 +402,10 @@  static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
     if (length < 0)
         return length;
     klv->length = length;
+    pos = avio_tell(pb);
+    if (pos > INT64_MAX - length)
+        return AVERROR_INVALIDDATA;
+    klv->next_klv = pos + length;
     return 0;
 }
 
@@ -3264,7 +3268,7 @@  static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
             IS_KLV_KEY(klv.key, mxf_avid_essence_element_key)) {
             int body_sid = find_body_sid_by_offset(mxf, klv.offset);
             int index = mxf_get_stream_index(s, &klv, body_sid);
-            int64_t next_ofs, next_klv;
+            int64_t next_ofs;
             AVStream *st;
 
             if (index < 0) {
@@ -3279,10 +3283,9 @@  static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
             if (s->streams[index]->discard == AVDISCARD_ALL)
                 goto skip;
 
-            next_klv = avio_tell(s->pb) + klv.length;
             next_ofs = mxf_set_current_edit_unit(mxf, klv.offset);
 
-            if (next_ofs >= 0 && next_klv > next_ofs) {
+            if (next_ofs >= 0 && klv.next_klv > next_ofs) {
                 /* if this check is hit then it's possible OPAtom was treated as OP1a
                  * truncate the packet since it's probably very large (>2 GiB is common) */
                 avpriv_request_sample(s,
@@ -3314,7 +3317,7 @@  static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
                 return ret;
 
             /* seek for truncated packets */
-            avio_seek(s->pb, next_klv, SEEK_SET);
+            avio_seek(s->pb, klv.next_klv, SEEK_SET);
 
             return 0;
         } else