[FFmpeg-devel,3/3] avformat/mov: Check for EOF in mov_read_meta()

Submitted by Michael Niedermayer on Aug. 30, 2019, 11:25 p.m.

Details

Message ID 20190830232503.17889-3-michael@niedermayer.cc
State Accepted
Commit 093d1f42507e07d9acb43a8a3135e4ebe3530fe2
Headers show

Commit Message

Michael Niedermayer Aug. 30, 2019, 11:25 p.m.
Fixes: Timeout (195sec -> 2ms)
Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mov.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

James Almer Aug. 30, 2019, 11:57 p.m.
On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
> Fixes: Timeout (195sec -> 2ms)
> Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mov.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 675b915906..46c544b61f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      while (atom.size > 8) {
> -        uint32_t tag = avio_rl32(pb);
> +        uint32_t tag;
> +        if (avio_feof(pb))
> +            return AVERROR_EOF;
> +        tag = avio_rl32(pb);
>          atom.size -= 4;
>          if (tag == MKTAG('h','d','l','r')) {
>              avio_seek(pb, -8, SEEK_CUR);

Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
instead, which is similar to the loop in mov_read_default.
Michael Niedermayer Aug. 31, 2019, 8:47 a.m.
On Fri, Aug 30, 2019 at 08:57:29PM -0300, James Almer wrote:
> On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
> > Fixes: Timeout (195sec -> 2ms)
> > Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mov.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 675b915906..46c544b61f 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> >      while (atom.size > 8) {
> > -        uint32_t tag = avio_rl32(pb);
> > +        uint32_t tag;
> > +        if (avio_feof(pb))
> > +            return AVERROR_EOF;
> > +        tag = avio_rl32(pb);
> >          atom.size -= 4;
> >          if (tag == MKTAG('h','d','l','r')) {
> >              avio_seek(pb, -8, SEEK_CUR);
> 
> Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
> instead, which is similar to the loop in mov_read_default.

Can do but why ?
the code in the patch returns an error if the atom is truncated
the change suggested does not return an error if the atom is truncated
on its own this doesnt sound better

Thanks


[...]
James Almer Aug. 31, 2019, 1:36 p.m.
On 8/31/2019 5:47 AM, Michael Niedermayer wrote:
> On Fri, Aug 30, 2019 at 08:57:29PM -0300, James Almer wrote:
>> On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
>>> Fixes: Timeout (195sec -> 2ms)
>>> Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavformat/mov.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 675b915906..46c544b61f 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>  {
>>>      while (atom.size > 8) {
>>> -        uint32_t tag = avio_rl32(pb);
>>> +        uint32_t tag;
>>> +        if (avio_feof(pb))
>>> +            return AVERROR_EOF;
>>> +        tag = avio_rl32(pb);
>>>          atom.size -= 4;
>>>          if (tag == MKTAG('h','d','l','r')) {
>>>              avio_seek(pb, -8, SEEK_CUR);
>>
>> Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
>> instead, which is similar to the loop in mov_read_default.
> 
> Can do but why ?
> the code in the patch returns an error if the atom is truncated
> the change suggested does not return an error if the atom is truncated
> on its own this doesnt sound better
> 
> Thanks

There's Marton's "avformat/utils: return pending IO error on EOF in
av_read_frame()" patch to check in generic code if avio_feof() != 0 is
an actual EOF or an IO error, so if you make this code here simply break
the loop, same as it's done in mov_read_default(), then said generic
code would be triggered and return the proper error code once the
current packet is done processing.
Nicolas George Aug. 31, 2019, 1:41 p.m.
James Almer (12019-08-31):
> There's Marton's "avformat/utils: return pending IO error on EOF in
> av_read_frame()" patch to check in generic code if avio_feof() != 0 is
> an actual EOF or an IO error, so if you make this code here simply break
> the loop, same as it's done in mov_read_default(), then said generic
> code would be triggered and return the proper error code once the
> current packet is done processing.

Please do not do that. It would be gaining very little time and lines of
code now at the cost of a likely maintenance nightmare later. Let us
keep the logic of the code local if it can be.

Regards,
Michael Niedermayer Sept. 15, 2019, 7:34 p.m.
On Sat, Aug 31, 2019 at 10:36:32AM -0300, James Almer wrote:
> On 8/31/2019 5:47 AM, Michael Niedermayer wrote:
> > On Fri, Aug 30, 2019 at 08:57:29PM -0300, James Almer wrote:
> >> On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
> >>> Fixes: Timeout (195sec -> 2ms)
> >>> Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavformat/mov.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>> index 675b915906..46c544b61f 100644
> >>> --- a/libavformat/mov.c
> >>> +++ b/libavformat/mov.c
> >>> @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>  {
> >>>      while (atom.size > 8) {
> >>> -        uint32_t tag = avio_rl32(pb);
> >>> +        uint32_t tag;
> >>> +        if (avio_feof(pb))
> >>> +            return AVERROR_EOF;
> >>> +        tag = avio_rl32(pb);
> >>>          atom.size -= 4;
> >>>          if (tag == MKTAG('h','d','l','r')) {
> >>>              avio_seek(pb, -8, SEEK_CUR);
> >>
> >> Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
> >> instead, which is similar to the loop in mov_read_default.
> > 
> > Can do but why ?
> > the code in the patch returns an error if the atom is truncated
> > the change suggested does not return an error if the atom is truncated
> > on its own this doesnt sound better
> > 
> > Thanks
> 
> There's Marton's "avformat/utils: return pending IO error on EOF in
> av_read_frame()" patch to check in generic code if avio_feof() != 0 is
> an actual EOF or an IO error, so if you make this code here simply break
> the loop, same as it's done in mov_read_default(), then said generic
> code would be triggered and return the proper error code once the
> current packet is done processing.

are you against the original patch in this thread ?
from reading this its not clear to me if you dislike the original
patch or not ?

thanks

[...]
James Almer Sept. 15, 2019, 7:35 p.m.
On 9/15/2019 4:34 PM, Michael Niedermayer wrote:
> On Sat, Aug 31, 2019 at 10:36:32AM -0300, James Almer wrote:
>> On 8/31/2019 5:47 AM, Michael Niedermayer wrote:
>>> On Fri, Aug 30, 2019 at 08:57:29PM -0300, James Almer wrote:
>>>> On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
>>>>> Fixes: Timeout (195sec -> 2ms)
>>>>> Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavformat/mov.c | 5 ++++-
>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>>> index 675b915906..46c544b61f 100644
>>>>> --- a/libavformat/mov.c
>>>>> +++ b/libavformat/mov.c
>>>>> @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>>  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>>  {
>>>>>      while (atom.size > 8) {
>>>>> -        uint32_t tag = avio_rl32(pb);
>>>>> +        uint32_t tag;
>>>>> +        if (avio_feof(pb))
>>>>> +            return AVERROR_EOF;
>>>>> +        tag = avio_rl32(pb);
>>>>>          atom.size -= 4;
>>>>>          if (tag == MKTAG('h','d','l','r')) {
>>>>>              avio_seek(pb, -8, SEEK_CUR);
>>>>
>>>> Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
>>>> instead, which is similar to the loop in mov_read_default.
>>>
>>> Can do but why ?
>>> the code in the patch returns an error if the atom is truncated
>>> the change suggested does not return an error if the atom is truncated
>>> on its own this doesnt sound better
>>>
>>> Thanks
>>
>> There's Marton's "avformat/utils: return pending IO error on EOF in
>> av_read_frame()" patch to check in generic code if avio_feof() != 0 is
>> an actual EOF or an IO error, so if you make this code here simply break
>> the loop, same as it's done in mov_read_default(), then said generic
>> code would be triggered and return the proper error code once the
>> current packet is done processing.
> 
> are you against the original patch in this thread ?
> from reading this its not clear to me if you dislike the original
> patch or not ?
> 
> thanks

No, I'm not.
Michael Niedermayer Sept. 17, 2019, 12:55 p.m.
On Sun, Sep 15, 2019 at 04:35:30PM -0300, James Almer wrote:
> On 9/15/2019 4:34 PM, Michael Niedermayer wrote:
> > On Sat, Aug 31, 2019 at 10:36:32AM -0300, James Almer wrote:
> >> On 8/31/2019 5:47 AM, Michael Niedermayer wrote:
> >>> On Fri, Aug 30, 2019 at 08:57:29PM -0300, James Almer wrote:
> >>>> On 8/30/2019 8:25 PM, Michael Niedermayer wrote:
> >>>>> Fixes: Timeout (195sec -> 2ms)
> >>>>> Fixes: 16735/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5090676403863552
> >>>>>
> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavformat/mov.c | 5 ++++-
> >>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>>>> index 675b915906..46c544b61f 100644
> >>>>> --- a/libavformat/mov.c
> >>>>> +++ b/libavformat/mov.c
> >>>>> @@ -4419,7 +4419,10 @@ static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>>>  static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>>>>  {
> >>>>>      while (atom.size > 8) {
> >>>>> -        uint32_t tag = avio_rl32(pb);
> >>>>> +        uint32_t tag;
> >>>>> +        if (avio_feof(pb))
> >>>>> +            return AVERROR_EOF;
> >>>>> +        tag = avio_rl32(pb);
> >>>>>          atom.size -= 4;
> >>>>>          if (tag == MKTAG('h','d','l','r')) {
> >>>>>              avio_seek(pb, -8, SEEK_CUR);
> >>>>
> >>>> Maybe do something like "while (atom.size > 8 && !avio_feof(pb))"
> >>>> instead, which is similar to the loop in mov_read_default.
> >>>
> >>> Can do but why ?
> >>> the code in the patch returns an error if the atom is truncated
> >>> the change suggested does not return an error if the atom is truncated
> >>> on its own this doesnt sound better
> >>>
> >>> Thanks
> >>
> >> There's Marton's "avformat/utils: return pending IO error on EOF in
> >> av_read_frame()" patch to check in generic code if avio_feof() != 0 is
> >> an actual EOF or an IO error, so if you make this code here simply break
> >> the loop, same as it's done in mov_read_default(), then said generic
> >> code would be triggered and return the proper error code once the
> >> current packet is done processing.
> > 
> > are you against the original patch in this thread ?
> > from reading this its not clear to me if you dislike the original
> > patch or not ?
> > 
> > thanks
> 
> No, I'm not.

ok, will apply

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 675b915906..46c544b61f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4419,7 +4419,10 @@  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     while (atom.size > 8) {
-        uint32_t tag = avio_rl32(pb);
+        uint32_t tag;
+        if (avio_feof(pb))
+            return AVERROR_EOF;
+        tag = avio_rl32(pb);
         atom.size -= 4;
         if (tag == MKTAG('h','d','l','r')) {
             avio_seek(pb, -8, SEEK_CUR);