[FFmpeg-devel,1/3] avformat/mxfdec: do not ignore bad size errors

Submitted by Marton Balint on Aug. 17, 2019, 7:41 p.m.

Details

Message ID 20190817194105.16492-1-cus@passwd.hu
State Accepted
Commit 6ee40dcb64c91cc9a4cb988408d8ed159dacdcfe
Headers show

Commit Message

Marton Balint Aug. 17, 2019, 7:41 p.m.
The return value was unintentionally lost after
00a2652df3bf25a27d174cc67ed508b5317cb115.

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

Comments

Tomas Härdin Aug. 18, 2019, 4:29 p.m.
lör 2019-08-17 klockan 21:41 +0200 skrev Marton Balint:
> The return value was unintentionally lost after
> 00a2652df3bf25a27d174cc67ed508b5317cb115.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index bb72fb9841..397f820b3f 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -3508,8 +3508,8 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>                  } else {
>                      if ((size = next_ofs - pos) <= 0) {
>                          av_log(s, AV_LOG_ERROR, "bad size: %"PRId64"\n", size);
> -                        ret = AVERROR_INVALIDDATA;
> -                        goto skip;
> +                        mxf->current_klv_data = (KLVPacket){{0}};
> +                        return AVERROR_INVALIDDATA;

Should maybe ask for a sample. Else looks OK

/Tomas
Marton Balint Aug. 21, 2019, 7:54 p.m.
On Sun, 18 Aug 2019, Tomas Härdin wrote:

> lör 2019-08-17 klockan 21:41 +0200 skrev Marton Balint:
>> The return value was unintentionally lost after
>> 00a2652df3bf25a27d174cc67ed508b5317cb115.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfdec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index bb72fb9841..397f820b3f 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -3508,8 +3508,8 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>>                  } else {
>>                      if ((size = next_ofs - pos) <= 0) {
>>                          av_log(s, AV_LOG_ERROR, "bad size: %"PRId64"\n", size);
>> -                        ret = AVERROR_INVALIDDATA;
>> -                        goto skip;
>> +                        mxf->current_klv_data = (KLVPacket){{0}};
>> +                        return AVERROR_INVALIDDATA;
>
> Should maybe ask for a sample. Else looks OK

I don't think this can happen with any of the valid files, so the text 
saying that ffmpeg is missing a feature probably won't be true. It is a 
lot more likely that the user has a broken file if this error is shown.

So I'd rather keep it as is.

Regards,
Marton
Tomas Härdin Aug. 22, 2019, 10:20 a.m.
ons 2019-08-21 klockan 21:54 +0200 skrev Marton Balint:
> 
> On Sun, 18 Aug 2019, Tomas Härdin wrote:
> 
> > lör 2019-08-17 klockan 21:41 +0200 skrev Marton Balint:
> > > The return value was unintentionally lost after
> > > 00a2652df3bf25a27d174cc67ed508b5317cb115.
> > > 
> > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > ---
> > >  libavformat/mxfdec.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index bb72fb9841..397f820b3f 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -3508,8 +3508,8 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >                  } else {
> > >                      if ((size = next_ofs - pos) <= 0) {
> > >                          av_log(s, AV_LOG_ERROR, "bad size: %"PRId64"\n", size);
> > > -                        ret = AVERROR_INVALIDDATA;
> > > -                        goto skip;
> > > +                        mxf->current_klv_data = (KLVPacket){{0}};
> > > +                        return AVERROR_INVALIDDATA;
> > 
> > Should maybe ask for a sample. Else looks OK
> 
> I don't think this can happen with any of the valid files, so the text 
> saying that ffmpeg is missing a feature probably won't be true. It is a 
> lot more likely that the user has a broken file if this error is shown.
> 
> So I'd rather keep it as is.

Right, the skip had me confused. This is fine of course

/Tomas
Marton Balint Aug. 22, 2019, 7:49 p.m.
On Thu, 22 Aug 2019, Tomas Härdin wrote:

> ons 2019-08-21 klockan 21:54 +0200 skrev Marton Balint:
>> 
>> On Sun, 18 Aug 2019, Tomas Härdin wrote:
>> 
>> > lör 2019-08-17 klockan 21:41 +0200 skrev Marton Balint:
>> > > The return value was unintentionally lost after
>> > > 00a2652df3bf25a27d174cc67ed508b5317cb115.
>> > > 
>> > > Signed-off-by: Marton Balint <cus@passwd.hu>
>> > > ---
>> > >  libavformat/mxfdec.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > > 
>> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> > > index bb72fb9841..397f820b3f 100644
>> > > --- a/libavformat/mxfdec.c
>> > > +++ b/libavformat/mxfdec.c
>> > > @@ -3508,8 +3508,8 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > >                  } else {
>> > >                      if ((size = next_ofs - pos) <= 0) {
>> > >                          av_log(s, AV_LOG_ERROR, "bad size: %"PRId64"\n", size);
>> > > -                        ret = AVERROR_INVALIDDATA;
>> > > -                        goto skip;
>> > > +                        mxf->current_klv_data = (KLVPacket){{0}};
>> > > +                        return AVERROR_INVALIDDATA;
>> > 
>> > Should maybe ask for a sample. Else looks OK
>> 
>> I don't think this can happen with any of the valid files, so the text 
>> saying that ffmpeg is missing a feature probably won't be true. It is a 
>> lot more likely that the user has a broken file if this error is shown.
>> 
>> So I'd rather keep it as is.
>
> Right, the skip had me confused. This is fine of course

Thanks, applied.

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index bb72fb9841..397f820b3f 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -3508,8 +3508,8 @@  static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
                 } else {
                     if ((size = next_ofs - pos) <= 0) {
                         av_log(s, AV_LOG_ERROR, "bad size: %"PRId64"\n", size);
-                        ret = AVERROR_INVALIDDATA;
-                        goto skip;
+                        mxf->current_klv_data = (KLVPacket){{0}};
+                        return AVERROR_INVALIDDATA;
                     }
                     // We must not overread, because the next edit unit might be in another KLV
                     if (size > max_data_size)