| Message ID | 20190830232503.17889-3-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 093d1f42507e07d9acb43a8a3135e4ebe3530fe2 |
| Headers | show |
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.
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 [...]
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.
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,
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 [...]
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.
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 [...]
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);
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(-)