[Libguestfs] [libnbd PATCH] common/utils/vector: Better overflow handling
Richard W.M. Jones
rjones at redhat.com
Mon Nov 8 21:30:20 UTC 2021
On Mon, Nov 08, 2021 at 10:08:59PM +0200, Nir Soffer wrote:
> On Mon, Nov 8, 2021 at 9:56 PM Eric Blake <eblake at redhat.com> wrote:
> >
> > Check newcap * itemsize for overflow prior to calling realloc, so that
> > we don't accidentally truncate an existing array. Set errno to ENOMEM
> > on all failure paths, rather than leaving it indeterminate on
> > overflow. The patch works by assuming a prerequisite that
> > v->cap*itemsize is always smaller than size_t.
> >
> > Fixes: 985dfa72ae (common/utils/vector.c: Optimize vector append)
> > ---
> >
> > Unless you see problems in this, I'll push this and also port it to
> > nbdkit.
> >
> > common/utils/vector.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/common/utils/vector.c b/common/utils/vector.c
> > index a4b43ce..c37f0c3 100644
> > --- a/common/utils/vector.c
> > +++ b/common/utils/vector.c
> > @@ -1,5 +1,5 @@
> > /* nbdkit
> > - * Copyright (C) 2018-2020 Red Hat Inc.
> > + * Copyright (C) 2018-2021 Red Hat Inc.
> > *
> > * Redistribution and use in source and binary forms, with or without
> > * modification, are permitted provided that the following conditions are
> > @@ -44,12 +44,14 @@ generic_vector_reserve (struct generic_vector *v, size_t n, size_t itemsize)
> > size_t reqcap, newcap;
> >
> > reqcap = v->cap + n;
> > - if (reqcap < v->cap)
> > + if (reqcap * itemsize < v->cap * itemsize) {
>
> Just to make sure I understand this correctly:
> v->cap * itemsize must be valid value since this is the current
> allocation, computed in a previous call to generic_vector_reserve...
>
> > + errno = ENOMEM;
> > return -1; /* overflow */
> > + }
> >
> > newcap = (v->cap * 3 + 1) / 2;
> >
> > - if (newcap < reqcap)
> > + if (newcap * itemsize < reqcap * itemsize)
>
> So reqcap * itemsize is valid because we passed the check above.
>
> > newcap = reqcap;
> >
> > newptr = realloc (v->ptr, newcap * itemsize);
>
> Looks right to me.
I agree too, it seems right here.
Guess it's one case where a proof checker could actually help :-)
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
More information about the Libguestfs
mailing list