Message ID | CAADho6NWzj-Ngfz4LO7hebZNvSUThe5hR8pMEo_TqdUwqZ0vgA@mail.gmail.com |
---|---|
State | Accepted |
Headers | show |
On 08/02/17 00:09, Matthew Wolenetz wrote: > From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001 > From: Matt Wolenetz <wolenetz@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@paulmehta.com > Reference https://crbug.com/643951 > > Signed-off-by: Matt Wolenetz <wolenetz@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
On Wed, Feb 08, 2017 at 01:25:58AM +0000, Mark Thompson wrote: > On 08/02/17 00:09, Matthew Wolenetz wrote: > > From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001 > > From: Matt Wolenetz <wolenetz@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@paulmehta.com > > Reference https://crbug.com/643951 > > > > Signed-off-by: Matt Wolenetz <wolenetz@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); that alone is not enough ill push a fix which appears to be sufficent, dont want to leave either of these open while its discussed whats the best solution if theres some issue in my fixes dont hesitate to improve or replace, ill go to bed after testing and pushing my fixes thx [...]
From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001 From: Matt Wolenetz <wolenetz@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@paulmehta.com Reference https://crbug.com/643951 Signed-off-by: Matt Wolenetz <wolenetz@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