| Message ID | 20180610103650.10155-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | bfa0b50441785871062b507ece63bfaf63f67dfd |
| Headers | show |
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
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
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
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
Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mxf.h | 1 + libavformat/mxfdec.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-)