| Message ID | 20201021223733.2563-6-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | c7a5face77878ef0169a56a46d4320a41d52d3b5 |
| Headers | show |
| Series | [FFmpeg-devel,1/9] avformat/segafilm: Check that there is a stream | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Michael Niedermayer: > Fixes: SEGV on unknown address 0x000000000000 > Fixes: 26482/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-4905102324006912 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/vividas.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > index 999c4a5d1c..6e93d96aef 100644 > --- a/libavformat/vividas.c > +++ b/libavformat/vividas.c > @@ -682,7 +682,7 @@ static int viv_read_packet(AVFormatContext *s, > return AVERROR_INVALIDDATA; > > ffio_read_varlen(pb); > - if (v_size > INT_MAX) > + if (v_size > INT_MAX || !v_size) > return AVERROR_INVALIDDATA; > ret = av_get_packet(pb, pkt, v_size); > if (ret < 0) > @@ -711,7 +711,7 @@ static int viv_read_packet(AVFormatContext *s, > } else { > uint64_t v_size = ffio_read_varlen(pb); > > - if (v_size > INT_MAX) > + if (v_size > INT_MAX || !v_size) > return AVERROR_INVALIDDATA; > ret = av_get_packet(pb, pkt, v_size); > if (ret < 0) > av_get_packet(pb, pkt, 0) should get a packet with pkt->data pointing to a zeroed buffer of size AV_INPUT_BUFFER_PADDING_SIZE. So accessing pkt->data[0] for the flags should not segfault (but it would set the wrong result: that it is a keyframe). So where is the segfault? - Andreas
On Thu, Oct 22, 2020 at 07:55:31AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: SEGV on unknown address 0x000000000000 > > Fixes: 26482/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-4905102324006912 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/vividas.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > > index 999c4a5d1c..6e93d96aef 100644 > > --- a/libavformat/vividas.c > > +++ b/libavformat/vividas.c > > @@ -682,7 +682,7 @@ static int viv_read_packet(AVFormatContext *s, > > return AVERROR_INVALIDDATA; > > > > ffio_read_varlen(pb); > > - if (v_size > INT_MAX) > > + if (v_size > INT_MAX || !v_size) > > return AVERROR_INVALIDDATA; > > ret = av_get_packet(pb, pkt, v_size); > > if (ret < 0) > > @@ -711,7 +711,7 @@ static int viv_read_packet(AVFormatContext *s, > > } else { > > uint64_t v_size = ffio_read_varlen(pb); > > > > - if (v_size > INT_MAX) > > + if (v_size > INT_MAX || !v_size) > > return AVERROR_INVALIDDATA; > > ret = av_get_packet(pb, pkt, v_size); > > if (ret < 0) > > > av_get_packet(pb, pkt, 0) should get a packet with pkt->data pointing to > a zeroed buffer of size AV_INPUT_BUFFER_PADDING_SIZE. So accessing > pkt->data[0] for the flags should not segfault (but it would set the > wrong result: that it is a keyframe). So where is the segfault? I tried to simulate this in a different demuxer: (different to make sure its not the file or demuxer that somehow affects this) --- a/libavformat/avidec.c +++ b/libavformat/avidec.c @@ -1446,9 +1446,10 @@ resync: if (size > ast->remaining) size = ast->remaining; avi->last_pkt_pos = avio_tell(pb); - err = av_get_packet(pb, pkt, size); + err = av_get_packet(pb, pkt, 0); if (err < 0) return err; + av_log(0,0, "P %p\n", pkt->data); size = err; if (ast->has_pal && pkt->size < (unsigned)INT_MAX / 2) { what is printed is: P (nil) [...]
Michael Niedermayer: > On Thu, Oct 22, 2020 at 07:55:31AM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> Fixes: SEGV on unknown address 0x000000000000 >>> Fixes: 26482/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-4905102324006912 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/vividas.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/vividas.c b/libavformat/vividas.c >>> index 999c4a5d1c..6e93d96aef 100644 >>> --- a/libavformat/vividas.c >>> +++ b/libavformat/vividas.c >>> @@ -682,7 +682,7 @@ static int viv_read_packet(AVFormatContext *s, >>> return AVERROR_INVALIDDATA; >>> >>> ffio_read_varlen(pb); >>> - if (v_size > INT_MAX) >>> + if (v_size > INT_MAX || !v_size) >>> return AVERROR_INVALIDDATA; >>> ret = av_get_packet(pb, pkt, v_size); >>> if (ret < 0) >>> @@ -711,7 +711,7 @@ static int viv_read_packet(AVFormatContext *s, >>> } else { >>> uint64_t v_size = ffio_read_varlen(pb); >>> >>> - if (v_size > INT_MAX) >>> + if (v_size > INT_MAX || !v_size) >>> return AVERROR_INVALIDDATA; >>> ret = av_get_packet(pb, pkt, v_size); >>> if (ret < 0) >>> >> av_get_packet(pb, pkt, 0) should get a packet with pkt->data pointing to >> a zeroed buffer of size AV_INPUT_BUFFER_PADDING_SIZE. So accessing >> pkt->data[0] for the flags should not segfault (but it would set the >> wrong result: that it is a keyframe). So where is the segfault? > > I tried to simulate this in a different demuxer: > (different to make sure its not the file or demuxer that somehow affects this) > --- a/libavformat/avidec.c > +++ b/libavformat/avidec.c > @@ -1446,9 +1446,10 @@ resync: > if (size > ast->remaining) > size = ast->remaining; > avi->last_pkt_pos = avio_tell(pb); > - err = av_get_packet(pb, pkt, size); > + err = av_get_packet(pb, pkt, 0); > if (err < 0) > return err; > + av_log(0,0, "P %p\n", pkt->data); > size = err; > > if (ast->has_pal && pkt->size < (unsigned)INT_MAX / 2) { > > what is printed is: > P (nil) > My bad: append_packet_chunked() unrefs the packet if pkt->size is zero before returning (without returning an error). So your patch seems to be the way to go. - Andreas
On Thu, Oct 22, 2020 at 06:55:37PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Thu, Oct 22, 2020 at 07:55:31AM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >>> Fixes: SEGV on unknown address 0x000000000000 > >>> Fixes: 26482/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-4905102324006912 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavformat/vividas.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavformat/vividas.c b/libavformat/vividas.c > >>> index 999c4a5d1c..6e93d96aef 100644 > >>> --- a/libavformat/vividas.c > >>> +++ b/libavformat/vividas.c > >>> @@ -682,7 +682,7 @@ static int viv_read_packet(AVFormatContext *s, > >>> return AVERROR_INVALIDDATA; > >>> > >>> ffio_read_varlen(pb); > >>> - if (v_size > INT_MAX) > >>> + if (v_size > INT_MAX || !v_size) > >>> return AVERROR_INVALIDDATA; > >>> ret = av_get_packet(pb, pkt, v_size); > >>> if (ret < 0) > >>> @@ -711,7 +711,7 @@ static int viv_read_packet(AVFormatContext *s, > >>> } else { > >>> uint64_t v_size = ffio_read_varlen(pb); > >>> > >>> - if (v_size > INT_MAX) > >>> + if (v_size > INT_MAX || !v_size) > >>> return AVERROR_INVALIDDATA; > >>> ret = av_get_packet(pb, pkt, v_size); > >>> if (ret < 0) > >>> > >> av_get_packet(pb, pkt, 0) should get a packet with pkt->data pointing to > >> a zeroed buffer of size AV_INPUT_BUFFER_PADDING_SIZE. So accessing > >> pkt->data[0] for the flags should not segfault (but it would set the > >> wrong result: that it is a keyframe). So where is the segfault? > > > > I tried to simulate this in a different demuxer: > > (different to make sure its not the file or demuxer that somehow affects this) > > --- a/libavformat/avidec.c > > +++ b/libavformat/avidec.c > > @@ -1446,9 +1446,10 @@ resync: > > if (size > ast->remaining) > > size = ast->remaining; > > avi->last_pkt_pos = avio_tell(pb); > > - err = av_get_packet(pb, pkt, size); > > + err = av_get_packet(pb, pkt, 0); > > if (err < 0) > > return err; > > + av_log(0,0, "P %p\n", pkt->data); > > size = err; > > > > if (ast->has_pal && pkt->size < (unsigned)INT_MAX / 2) { > > > > what is printed is: > > P (nil) > > > My bad: append_packet_chunked() unrefs the packet if pkt->size is zero > before returning (without returning an error). So your patch seems to be > the way to go. ok, will apply thx [...]
diff --git a/libavformat/vividas.c b/libavformat/vividas.c index 999c4a5d1c..6e93d96aef 100644 --- a/libavformat/vividas.c +++ b/libavformat/vividas.c @@ -682,7 +682,7 @@ static int viv_read_packet(AVFormatContext *s, return AVERROR_INVALIDDATA; ffio_read_varlen(pb); - if (v_size > INT_MAX) + if (v_size > INT_MAX || !v_size) return AVERROR_INVALIDDATA; ret = av_get_packet(pb, pkt, v_size); if (ret < 0) @@ -711,7 +711,7 @@ static int viv_read_packet(AVFormatContext *s, } else { uint64_t v_size = ffio_read_varlen(pb); - if (v_size > INT_MAX) + if (v_size > INT_MAX || !v_size) return AVERROR_INVALIDDATA; ret = av_get_packet(pb, pkt, v_size); if (ret < 0)
Fixes: SEGV on unknown address 0x000000000000 Fixes: 26482/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-4905102324006912 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/vividas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)