[FFmpeg-devel] [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid

[Mark Thompson]

On 08/02/17 00:09, Matthew Wolenetz wrote:
> From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz <wolenetz at google.com>
> Date: Wed, 14 Dec 2016 15:26:19 -0800
> Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid
> 
> Core of patch is from paul at paulmehta.com
> Reference https://crbug.com/643951
> 
> Signed-off-by: Matt Wolenetz <wolenetz at chromium.org>
> ---
>  libavformat/mov.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 6fd43a0a4e..93aece510c 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4849,6 +4849,8 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          uint8_t *buffer;
>          size_t len = atom.size - sizeof(uuid);
>          if (c->export_xmp) {
> +            if (len >= SIZE_MAX)
> +                return AVERROR_INVALIDDATA;
>              buffer = av_mallocz(len + 1);
>              if (!buffer) {
>                  return AVERROR(ENOMEM);
> -- 
> 2.11.0.483.g087da7b7c-goog

I don't know precisely what this is trying to do, but it doesn't look very sensible to me (I can't see any context because your reference link is blocked behind some sort of login page).

Since SIZE_MAX is the largest size_t (i.e. -1), only len == SIZE_MAX can satisfy the test so it is actually "if (atom.size == sizeof(uuid) - 1)", and if atom.size is something less than sizeof(uuid) - 1 then you are passing a silly value to av_mallocz().

I admit that it doesn't actually invoke undefined behaviour in this case, but the general rules for overflow checks to avoid any surprises are:
(a) Always put the check before the operation which might overflow.
(b) Always express the check in a way which cannot itself overflow.

So, I think you would be better off writing:

size_t len;
if (atom.size < sizeof(uuid))
    return AVERROR_INVALIDDATA;
len = atom.size - sizeof(uuid);

(If there is additional information hidden behind the link which renders my comments invalid then please do share it directly.)

- Mark