| Message ID | AM7PR03MB6660E96FCB508BFD1C1A04558FE69@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | c2d853c1aae22bbc7d9905c43a9f16cb2ba3ba33 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mov: Fix crash with too big STSZ atoms | 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 |
On Sat, Jul 24, 2021 at 6:09 AM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > mov_read_stsz() did not ensure that every bit of a buffer is addressable > by an int as is required by the get_bits API, leading to a crash in > ticket #9344. Fix this by restricting the size more thoroughly. > > The file from said ticket will then be considered invalid; in the > future, we might read and process the data in chunks to actually support > such files. > > Fixes ticket #9344. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > The commit message is written as if it were certain that this > indeed fixes the ticket, despite me not knowing it yet (as the sample > in question is not public). > The above is intended as a quick fix that is easy to backport; > supporting such files can be done later. > > libavformat/mov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 3fc5a1e8ab..e0d805b07b 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext > *pb, MOVAtom atom) > > if (!entries) > return 0; > - if (entries >= (UINT_MAX - 4) / field_size) > + if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) / > field_size) > return AVERROR_INVALIDDATA; > if (sc->sample_sizes) > av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n"); > -- > 2.30.2 > Is so big bit buffer really needed? Why not check use init_get_bits8 directly and thus depends on its implementation directly? > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Paul B Mahol: > On Sat, Jul 24, 2021 at 6:09 AM Andreas Rheinhardt < > andreas.rheinhardt@outlook.com> wrote: > >> mov_read_stsz() did not ensure that every bit of a buffer is addressable >> by an int as is required by the get_bits API, leading to a crash in >> ticket #9344. Fix this by restricting the size more thoroughly. >> >> The file from said ticket will then be considered invalid; in the >> future, we might read and process the data in chunks to actually support >> such files. >> >> Fixes ticket #9344. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> The commit message is written as if it were certain that this >> indeed fixes the ticket, despite me not knowing it yet (as the sample >> in question is not public). >> The above is intended as a quick fix that is easy to backport; >> supporting such files can be done later. >> >> libavformat/mov.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index 3fc5a1e8ab..e0d805b07b 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext >> *pb, MOVAtom atom) >> >> if (!entries) >> return 0; >> - if (entries >= (UINT_MAX - 4) / field_size) >> + if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) / >> field_size) >> return AVERROR_INVALIDDATA; >> if (sc->sample_sizes) >> av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n"); >> -- >> 2.30.2 >> > > Is so big bit buffer really needed? > No, such a big buffer is never needed, as one can read in smaller chunks (this would actually support the files that #9344 is about). I wanted to implement this, but I never got around to investigate what a sane chunk size is (one that avoids reading multiple times for ordinary files). Is it 1MB? 4MB? > Why not check use init_get_bits8 directly and thus depends on its > implementation directly? > Because then we have already allocated a buffer. Checking before the allocation avoids that. - Andreas
diff --git a/libavformat/mov.c b/libavformat/mov.c index 3fc5a1e8ab..e0d805b07b 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext *pb, MOVAtom atom) if (!entries) return 0; - if (entries >= (UINT_MAX - 4) / field_size) + if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) / field_size) return AVERROR_INVALIDDATA; if (sc->sample_sizes) av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n");
mov_read_stsz() did not ensure that every bit of a buffer is addressable by an int as is required by the get_bits API, leading to a crash in ticket #9344. Fix this by restricting the size more thoroughly. The file from said ticket will then be considered invalid; in the future, we might read and process the data in chunks to actually support such files. Fixes ticket #9344. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- The commit message is written as if it were certain that this indeed fixes the ticket, despite me not knowing it yet (as the sample in question is not public). The above is intended as a quick fix that is easy to backport; supporting such files can be done later. libavformat/mov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)