| Message ID | 20200507103826.3278-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | cd74af14162c803f18e90bb12b52135e893d990c |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mpegts: Shuffle avio_seek | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting Michael Niedermayer (2020-05-07 12:38:26) > This avoids accessing an old, no longer valid buffer. > Fixes: out of array access > Fixes: crash_audio-2020 > > Found-by: le wu <shoulewoba@gmail.com> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mpegts.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index 0833d62ea5..a065c61c40 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -2881,15 +2881,16 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren > AVIOContext *pb = s->pb; > int c, i; > uint64_t pos = avio_tell(pb); > - > - avio_seek(pb, -FFMIN(seekback, pos), SEEK_CUR); > + int64_t back = FFMIN(seekback, pos); > > //Special case for files like 01c56b0dc1.ts > if (current_packet[0] == 0x80 && current_packet[12] == 0x47) { > - avio_seek(pb, 12, SEEK_CUR); > + avio_seek(pb, 12 - back, SEEK_CUR); > return 0; > } > > + avio_seek(pb, -back, SEEK_CUR); > + This seems pretty non-obvious - why would ordering seeks in a specific way result in invalid memorry access?
On Thu, May 07, 2020 at 02:09:43PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-05-07 12:38:26) > > This avoids accessing an old, no longer valid buffer. > > Fixes: out of array access > > Fixes: crash_audio-2020 > > > > Found-by: le wu <shoulewoba@gmail.com> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/mpegts.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index 0833d62ea5..a065c61c40 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -2881,15 +2881,16 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren > > AVIOContext *pb = s->pb; > > int c, i; > > uint64_t pos = avio_tell(pb); > > - > > - avio_seek(pb, -FFMIN(seekback, pos), SEEK_CUR); > > + int64_t back = FFMIN(seekback, pos); > > > > //Special case for files like 01c56b0dc1.ts > > if (current_packet[0] == 0x80 && current_packet[12] == 0x47) { > > - avio_seek(pb, 12, SEEK_CUR); > > + avio_seek(pb, 12 - back, SEEK_CUR); > > return 0; > > } > > > > + avio_seek(pb, -back, SEEK_CUR); > > + > > This seems pretty non-obvious - why would ordering seeks in a specific > way result in invalid memorry access? because current_packet in one case points to the avio internal buffer and doing any "state changing" avio could change that buffer. so all accesses to the buffer must happen before any avio seeks this issue in fact was not reproducable in master but some release branches it just seems master is affected too. For reproduction a custom http server may be required, i did had some initial difficulty with reproduction even on the affected release branch ... thx [...]
On Thu, 7 May 2020, Michael Niedermayer wrote: > On Thu, May 07, 2020 at 02:09:43PM +0200, Anton Khirnov wrote: >> Quoting Michael Niedermayer (2020-05-07 12:38:26) >>> This avoids accessing an old, no longer valid buffer. >>> Fixes: out of array access >>> Fixes: crash_audio-2020 >>> >>> Found-by: le wu <shoulewoba@gmail.com> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/mpegts.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c >>> index 0833d62ea5..a065c61c40 100644 >>> --- a/libavformat/mpegts.c >>> +++ b/libavformat/mpegts.c >>> @@ -2881,15 +2881,16 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren >>> AVIOContext *pb = s->pb; >>> int c, i; >>> uint64_t pos = avio_tell(pb); >>> - >>> - avio_seek(pb, -FFMIN(seekback, pos), SEEK_CUR); >>> + int64_t back = FFMIN(seekback, pos); >>> >>> //Special case for files like 01c56b0dc1.ts >>> if (current_packet[0] == 0x80 && current_packet[12] == 0x47) { >>> - avio_seek(pb, 12, SEEK_CUR); >>> + avio_seek(pb, 12 - back, SEEK_CUR); >>> return 0; >>> } >>> >>> + avio_seek(pb, -back, SEEK_CUR); >>> + >> >> This seems pretty non-obvious - why would ordering seeks in a specific >> way result in invalid memorry access? > > because current_packet in one case points to the avio internal buffer > and doing any "state changing" avio could change that buffer. > so all accesses to the buffer must happen before any avio seeks Yuck. Patch LGTM though. Thanks, Marton
On Sun, May 17, 2020 at 02:11:19PM +0200, Marton Balint wrote: > > > On Thu, 7 May 2020, Michael Niedermayer wrote: > > >On Thu, May 07, 2020 at 02:09:43PM +0200, Anton Khirnov wrote: > >>Quoting Michael Niedermayer (2020-05-07 12:38:26) > >>>This avoids accessing an old, no longer valid buffer. > >>>Fixes: out of array access > >>>Fixes: crash_audio-2020 > >>> > >>>Found-by: le wu <shoulewoba@gmail.com> > >>>Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>--- > >>> libavformat/mpegts.c | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>>diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > >>>index 0833d62ea5..a065c61c40 100644 > >>>--- a/libavformat/mpegts.c > >>>+++ b/libavformat/mpegts.c > >>>@@ -2881,15 +2881,16 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren > >>> AVIOContext *pb = s->pb; > >>> int c, i; > >>> uint64_t pos = avio_tell(pb); > >>>- > >>>- avio_seek(pb, -FFMIN(seekback, pos), SEEK_CUR); > >>>+ int64_t back = FFMIN(seekback, pos); > >>> > >>> //Special case for files like 01c56b0dc1.ts > >>> if (current_packet[0] == 0x80 && current_packet[12] == 0x47) { > >>>- avio_seek(pb, 12, SEEK_CUR); > >>>+ avio_seek(pb, 12 - back, SEEK_CUR); > >>> return 0; > >>> } > >>> > >>>+ avio_seek(pb, -back, SEEK_CUR); > >>>+ > >> > >>This seems pretty non-obvious - why would ordering seeks in a specific > >>way result in invalid memorry access? > > > >because current_packet in one case points to the avio internal buffer > >and doing any "state changing" avio could change that buffer. > >so all accesses to the buffer must happen before any avio seeks > > Yuck. Patch LGTM though. will apply thx [...]
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index 0833d62ea5..a065c61c40 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -2881,15 +2881,16 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren AVIOContext *pb = s->pb; int c, i; uint64_t pos = avio_tell(pb); - - avio_seek(pb, -FFMIN(seekback, pos), SEEK_CUR); + int64_t back = FFMIN(seekback, pos); //Special case for files like 01c56b0dc1.ts if (current_packet[0] == 0x80 && current_packet[12] == 0x47) { - avio_seek(pb, 12, SEEK_CUR); + avio_seek(pb, 12 - back, SEEK_CUR); return 0; } + avio_seek(pb, -back, SEEK_CUR); + for (i = 0; i < ts->resync_size; i++) { c = avio_r8(pb); if (avio_feof(pb))
This avoids accessing an old, no longer valid buffer. Fixes: out of array access Fixes: crash_audio-2020 Found-by: le wu <shoulewoba@gmail.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mpegts.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)