| Message ID | 20240402031800.7159-3-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/mov: take into account the first eight bytes in the keys atom | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/mov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index a935ef7326..9fca402896 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -5025,7 +5025,7 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom) > avio_skip(pb, 4); > count = avio_rb32(pb); > atom.size -= 8; > - if (count > UINT_MAX / sizeof(*c->meta_keys) - 1) { > + if (count + 1LL > UINT_MAX / sizeof(*c->meta_keys)) { > av_log(c->fc, AV_LOG_ERROR, > "The 'keys' atom with the invalid key count: %"PRIu32"\n", count); > return AVERROR_INVALIDDATA; What is supposed to be wrong here in the first place? The only thing I can think of is the case in which sizeof(*c->meta_keys) is > UINT_MAX, in which case the rhs would wrap around. But I don't think that is what you meant given that sizeof(*c->meta_keys) == sizeof(char*). Anyway, a simpler check that works even if sizeof(*c->meta_keys) were insanely large is "count >= UINT_MAX / sizeof(*c->meta_keys)". - Andreas
On 4/2/2024 12:30 AM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/mov.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index a935ef7326..9fca402896 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -5025,7 +5025,7 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom) >> avio_skip(pb, 4); >> count = avio_rb32(pb); >> atom.size -= 8; >> - if (count > UINT_MAX / sizeof(*c->meta_keys) - 1) { >> + if (count + 1LL > UINT_MAX / sizeof(*c->meta_keys)) { >> av_log(c->fc, AV_LOG_ERROR, >> "The 'keys' atom with the invalid key count: %"PRIu32"\n", count); >> return AVERROR_INVALIDDATA; > > What is supposed to be wrong here in the first place? The only thing I > can think of is the case in which sizeof(*c->meta_keys) is > UINT_MAX, > in which case the rhs would wrap around. But I don't think that is what > you meant given that sizeof(*c->meta_keys) == sizeof(char*). > Anyway, a simpler check that works even if sizeof(*c->meta_keys) were > insanely large is "count >= UINT_MAX / sizeof(*c->meta_keys)". I misread the check, so there's nothing wrong with it. But I'll apply that suggestion of yours in any case since it's simpler than the current one.
diff --git a/libavformat/mov.c b/libavformat/mov.c index a935ef7326..9fca402896 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -5025,7 +5025,7 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom) avio_skip(pb, 4); count = avio_rb32(pb); atom.size -= 8; - if (count > UINT_MAX / sizeof(*c->meta_keys) - 1) { + if (count + 1LL > UINT_MAX / sizeof(*c->meta_keys)) { av_log(c->fc, AV_LOG_ERROR, "The 'keys' atom with the invalid key count: %"PRIu32"\n", count); return AVERROR_INVALIDDATA;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/mov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)