| Message ID | 20210624205724.18711-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | d115eec97929e23fd1b06df2d95f48cf5000eb87 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/matroskadec: Reset state also on failure in matroska_reset_status() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Michael Niedermayer (12021-06-24): > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long' > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/sbgdec.c | 3 +++ > 1 file changed, 3 insertions(+) Ok. Regards,
Michael Niedermayer: > The calling code does not handle failures and will fail with assertion failures later. > Seeking can always fail even when the position was previously read. > > Fixes: Assertion failure > Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/matroskadec.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 356a02339c..a0e6e0cf8b 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); > static int matroska_reset_status(MatroskaDemuxContext *matroska, > uint32_t id, int64_t position) > { > + int64_t err = 0; > if (position >= 0) { > - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > - if (err < 0) > - return err; > - } > + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > + if (err > 0) > + err = 0; > + } else > + position = avio_tell(matroska->ctx->pb); > > matroska->current_id = id; > matroska->num_levels = 1; > matroska->unknown_count = 0; > - matroska->resync_pos = avio_tell(matroska->ctx->pb); > + matroska->resync_pos = position; > if (id) > matroska->resync_pos -= (av_log2(id) + 7) / 8; > > - return 0; > + return err; The changes here will make the demuxer update its internal state as if it had seeked to its target level-1-element, even though it didn't. Is this really good? > } > > static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) > @@ -1873,6 +1875,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, > uint32_t saved_id = matroska->current_id; > int64_t before_pos = avio_tell(matroska->ctx->pb); > int ret = 0; > + int ret2; > > /* seek */ > if (avio_seek(matroska->ctx->pb, pos, SEEK_SET) == pos) { > @@ -1897,7 +1900,9 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, > } > /* Seek back - notice that in all instances where this is used > * it is safe to set the level to 1. */ > - matroska_reset_status(matroska, saved_id, before_pos); > + ret2 = matroska_reset_status(matroska, saved_id, before_pos); > + if (ret >= 0) > + ret = ret2; > > return ret; > } >
On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > The calling code does not handle failures and will fail with assertion failures later. > > Seeking can always fail even when the position was previously read. > > > > Fixes: Assertion failure > > Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/matroskadec.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > > index 356a02339c..a0e6e0cf8b 100644 > > --- a/libavformat/matroskadec.c > > +++ b/libavformat/matroskadec.c > > @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); > > static int matroska_reset_status(MatroskaDemuxContext *matroska, > > uint32_t id, int64_t position) > > { > > + int64_t err = 0; > > if (position >= 0) { > > - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > > - if (err < 0) > > - return err; > > - } > > + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > > + if (err > 0) > > + err = 0; > > + } else > > + position = avio_tell(matroska->ctx->pb); > > > > matroska->current_id = id; > > matroska->num_levels = 1; > > matroska->unknown_count = 0; > > - matroska->resync_pos = avio_tell(matroska->ctx->pb); > > + matroska->resync_pos = position; > > if (id) > > matroska->resync_pos -= (av_log2(id) + 7) / 8; > > > > - return 0; > > + return err; > > The changes here will make the demuxer update its internal state as if > it had seeked to its target level-1-element, even though it didn't. Is > this really good? I dont know. Ive not seen this issue happen in reality just in a fuzzer environment. [...]
Michael Niedermayer: > On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> The calling code does not handle failures and will fail with assertion failures later. >>> Seeking can always fail even when the position was previously read. >>> >>> Fixes: Assertion failure >>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/matroskadec.c | 19 ++++++++++++------- >>> 1 file changed, 12 insertions(+), 7 deletions(-) >>> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>> index 356a02339c..a0e6e0cf8b 100644 >>> --- a/libavformat/matroskadec.c >>> +++ b/libavformat/matroskadec.c >>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); >>> static int matroska_reset_status(MatroskaDemuxContext *matroska, >>> uint32_t id, int64_t position) >>> { >>> + int64_t err = 0; >>> if (position >= 0) { >>> - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); >>> - if (err < 0) >>> - return err; >>> - } >>> + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); >>> + if (err > 0) >>> + err = 0; >>> + } else >>> + position = avio_tell(matroska->ctx->pb); >>> >>> matroska->current_id = id; >>> matroska->num_levels = 1; >>> matroska->unknown_count = 0; >>> - matroska->resync_pos = avio_tell(matroska->ctx->pb); >>> + matroska->resync_pos = position; >>> if (id) >>> matroska->resync_pos -= (av_log2(id) + 7) / 8; >>> >>> - return 0; >>> + return err; >> >> The changes here will make the demuxer update its internal state as if >> it had seeked to its target level-1-element, even though it didn't. Is >> this really good? > > I dont know. > Ive not seen this issue happen in reality just in a fuzzer > environment. > Can you send me this sample (with instructions how to reproduce it if necessary)? - Andreas
On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >>> The calling code does not handle failures and will fail with assertion failures later. > >>> Seeking can always fail even when the position was previously read. > >>> > >>> Fixes: Assertion failure > >>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavformat/matroskadec.c | 19 ++++++++++++------- > >>> 1 file changed, 12 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > >>> index 356a02339c..a0e6e0cf8b 100644 > >>> --- a/libavformat/matroskadec.c > >>> +++ b/libavformat/matroskadec.c > >>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); > >>> static int matroska_reset_status(MatroskaDemuxContext *matroska, > >>> uint32_t id, int64_t position) > >>> { > >>> + int64_t err = 0; > >>> if (position >= 0) { > >>> - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > >>> - if (err < 0) > >>> - return err; > >>> - } > >>> + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > >>> + if (err > 0) > >>> + err = 0; > >>> + } else > >>> + position = avio_tell(matroska->ctx->pb); > >>> > >>> matroska->current_id = id; > >>> matroska->num_levels = 1; > >>> matroska->unknown_count = 0; > >>> - matroska->resync_pos = avio_tell(matroska->ctx->pb); > >>> + matroska->resync_pos = position; > >>> if (id) > >>> matroska->resync_pos -= (av_log2(id) + 7) / 8; > >>> > >>> - return 0; > >>> + return err; > >> > >> The changes here will make the demuxer update its internal state as if > >> it had seeked to its target level-1-element, even though it didn't. Is > >> this really good? > > > > I dont know. > > Ive not seen this issue happen in reality just in a fuzzer > > environment. > > > > Can you send me this sample (with instructions how to reproduce it if > necessary)? This seems still "crashing" according to the tracker has this been fixed ? thx [...]
Michael Niedermayer: > On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: >>>> Michael Niedermayer: >>>>> The calling code does not handle failures and will fail with assertion failures later. >>>>> Seeking can always fail even when the position was previously read. >>>>> >>>>> Fixes: Assertion failure >>>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 >>>>> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>> --- >>>>> libavformat/matroskadec.c | 19 ++++++++++++------- >>>>> 1 file changed, 12 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>>>> index 356a02339c..a0e6e0cf8b 100644 >>>>> --- a/libavformat/matroskadec.c >>>>> +++ b/libavformat/matroskadec.c >>>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); >>>>> static int matroska_reset_status(MatroskaDemuxContext *matroska, >>>>> uint32_t id, int64_t position) >>>>> { >>>>> + int64_t err = 0; >>>>> if (position >= 0) { >>>>> - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); >>>>> - if (err < 0) >>>>> - return err; >>>>> - } >>>>> + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); >>>>> + if (err > 0) >>>>> + err = 0; >>>>> + } else >>>>> + position = avio_tell(matroska->ctx->pb); >>>>> >>>>> matroska->current_id = id; >>>>> matroska->num_levels = 1; >>>>> matroska->unknown_count = 0; >>>>> - matroska->resync_pos = avio_tell(matroska->ctx->pb); >>>>> + matroska->resync_pos = position; >>>>> if (id) >>>>> matroska->resync_pos -= (av_log2(id) + 7) / 8; >>>>> >>>>> - return 0; >>>>> + return err; >>>> >>>> The changes here will make the demuxer update its internal state as if >>>> it had seeked to its target level-1-element, even though it didn't. Is >>>> this really good? >>> >>> I dont know. >>> Ive not seen this issue happen in reality just in a fuzzer >>> environment. >>> >> >> Can you send me this sample (with instructions how to reproduce it if >> necessary)? > > This seems still "crashing" according to the tracker > has this been fixed ? > No, I have not found time to look at it. Proceed as you wish. - Andreas
On Mon, Sep 13, 2021 at 10:57:41PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >>> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: > >>>> Michael Niedermayer: > >>>>> The calling code does not handle failures and will fail with assertion failures later. > >>>>> Seeking can always fail even when the position was previously read. > >>>>> > >>>>> Fixes: Assertion failure > >>>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > >>>>> > >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>>> --- > >>>>> libavformat/matroskadec.c | 19 ++++++++++++------- > >>>>> 1 file changed, 12 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > >>>>> index 356a02339c..a0e6e0cf8b 100644 > >>>>> --- a/libavformat/matroskadec.c > >>>>> +++ b/libavformat/matroskadec.c > >>>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); > >>>>> static int matroska_reset_status(MatroskaDemuxContext *matroska, > >>>>> uint32_t id, int64_t position) > >>>>> { > >>>>> + int64_t err = 0; > >>>>> if (position >= 0) { > >>>>> - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > >>>>> - if (err < 0) > >>>>> - return err; > >>>>> - } > >>>>> + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > >>>>> + if (err > 0) > >>>>> + err = 0; > >>>>> + } else > >>>>> + position = avio_tell(matroska->ctx->pb); > >>>>> > >>>>> matroska->current_id = id; > >>>>> matroska->num_levels = 1; > >>>>> matroska->unknown_count = 0; > >>>>> - matroska->resync_pos = avio_tell(matroska->ctx->pb); > >>>>> + matroska->resync_pos = position; > >>>>> if (id) > >>>>> matroska->resync_pos -= (av_log2(id) + 7) / 8; > >>>>> > >>>>> - return 0; > >>>>> + return err; > >>>> > >>>> The changes here will make the demuxer update its internal state as if > >>>> it had seeked to its target level-1-element, even though it didn't. Is > >>>> this really good? > >>> > >>> I dont know. > >>> Ive not seen this issue happen in reality just in a fuzzer > >>> environment. > >>> > >> > >> Can you send me this sample (with instructions how to reproduce it if > >> necessary)? > > > > This seems still "crashing" according to the tracker > > has this been fixed ? > > > > No, I have not found time to look at it. Proceed as you wish. well, i will look at other issues first then, maybe someone else will fix this ... thx [...]
On Tue, Sep 14, 2021 at 06:13:43PM +0200, Michael Niedermayer wrote: > On Mon, Sep 13, 2021 at 10:57:41PM +0200, Andreas Rheinhardt wrote: > > Michael Niedermayer: > > > On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote: > > >> Michael Niedermayer: > > >>> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote: > > >>>> Michael Niedermayer: > > >>>>> The calling code does not handle failures and will fail with assertion failures later. > > >>>>> Seeking can always fail even when the position was previously read. > > >>>>> > > >>>>> Fixes: Assertion failure > > >>>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > > >>>>> > > >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >>>>> --- > > >>>>> libavformat/matroskadec.c | 19 ++++++++++++------- > > >>>>> 1 file changed, 12 insertions(+), 7 deletions(-) > > >>>>> > > >>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > > >>>>> index 356a02339c..a0e6e0cf8b 100644 > > >>>>> --- a/libavformat/matroskadec.c > > >>>>> +++ b/libavformat/matroskadec.c > > >>>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); > > >>>>> static int matroska_reset_status(MatroskaDemuxContext *matroska, > > >>>>> uint32_t id, int64_t position) > > >>>>> { > > >>>>> + int64_t err = 0; > > >>>>> if (position >= 0) { > > >>>>> - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > > >>>>> - if (err < 0) > > >>>>> - return err; > > >>>>> - } > > >>>>> + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); > > >>>>> + if (err > 0) > > >>>>> + err = 0; > > >>>>> + } else > > >>>>> + position = avio_tell(matroska->ctx->pb); > > >>>>> > > >>>>> matroska->current_id = id; > > >>>>> matroska->num_levels = 1; > > >>>>> matroska->unknown_count = 0; > > >>>>> - matroska->resync_pos = avio_tell(matroska->ctx->pb); > > >>>>> + matroska->resync_pos = position; > > >>>>> if (id) > > >>>>> matroska->resync_pos -= (av_log2(id) + 7) / 8; > > >>>>> > > >>>>> - return 0; > > >>>>> + return err; > > >>>> > > >>>> The changes here will make the demuxer update its internal state as if > > >>>> it had seeked to its target level-1-element, even though it didn't. Is > > >>>> this really good? > > >>> > > >>> I dont know. > > >>> Ive not seen this issue happen in reality just in a fuzzer > > >>> environment. > > >>> > > >> > > >> Can you send me this sample (with instructions how to reproduce it if > > >> necessary)? > > > > > > This seems still "crashing" according to the tracker > > > has this been fixed ? > > > > > > > No, I have not found time to look at it. Proceed as you wish. > > well, i will look at other issues first then, maybe someone else > will fix this ... noone ? :( if noone wants to look into this then ill apply the original patch thx [...]
On Thu, Jun 24, 2021 at 10:57:22PM +0200, Michael Niedermayer wrote: > The calling code does not handle failures and will fail with assertion failures later. > Seeking can always fail even when the position was previously read. > > Fixes: Assertion failure > Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/matroskadec.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) will apply as no other solution was implemented by anyone thx [...]
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 356a02339c..a0e6e0cf8b 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s); static int matroska_reset_status(MatroskaDemuxContext *matroska, uint32_t id, int64_t position) { + int64_t err = 0; if (position >= 0) { - int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET); - if (err < 0) - return err; - } + err = avio_seek(matroska->ctx->pb, position, SEEK_SET); + if (err > 0) + err = 0; + } else + position = avio_tell(matroska->ctx->pb); matroska->current_id = id; matroska->num_levels = 1; matroska->unknown_count = 0; - matroska->resync_pos = avio_tell(matroska->ctx->pb); + matroska->resync_pos = position; if (id) matroska->resync_pos -= (av_log2(id) + 7) / 8; - return 0; + return err; } static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos) @@ -1873,6 +1875,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, uint32_t saved_id = matroska->current_id; int64_t before_pos = avio_tell(matroska->ctx->pb); int ret = 0; + int ret2; /* seek */ if (avio_seek(matroska->ctx->pb, pos, SEEK_SET) == pos) { @@ -1897,7 +1900,9 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, } /* Seek back - notice that in all instances where this is used * it is safe to set the level to 1. */ - matroska_reset_status(matroska, saved_id, before_pos); + ret2 = matroska_reset_status(matroska, saved_id, before_pos); + if (ret >= 0) + ret = ret2; return ret; }
The calling code does not handle failures and will fail with assertion failures later. Seeking can always fail even when the position was previously read. Fixes: Assertion failure Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/matroskadec.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)