[FFmpeg-devel] [PATCH 1/3] avformat/mov: take into account the first eight bytes in the keys atom

[FFmpeg-devel] [PATCH 1/3] avformat/mov: take into account the first eight bytes in the keys atom

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/mov.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2b7ddc516c..3273e2e89b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5024,6 +5024,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) {
         av_log(c->fc, AV_LOG_ERROR,
                "The 'keys' atom with the invalid key count: %"PRIu32"\n", count);
-- 
2.44.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/3] avformat/mov: don't read key_size bytes twice in the keys atom

[James Almer]

We only support mdta as type, yet we were not skipping other types,
but rather reading key_size worth of bytes twice per entry.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/mov.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3273e2e89b..a935ef7326 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5049,6 +5049,7 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         key_size -= 8;
         if (type != MKTAG('m','d','t','a')) {
             avio_skip(pb, key_size);
+            continue;
         }
         c->meta_keys[i] = av_mallocz(key_size + 1);
         if (!c->meta_keys[i])
-- 
2.44.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/3] avformat/mov: fix the entry count overflow check in the keys atom

[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;
-- 
2.44.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/3] avformat/mov: fix the entry count overflow check in the keys atom

[Andreas Rheinhardt]

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

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/3] avformat/mov: fix the entry count overflow check in the keys atom

[James Almer]

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.
_______________________________________________
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".