Re: [Libguestfs] [PATCH nbdkit 2/2] common: Add checked-overflow macros and use for safe vector extension

--- common/include/Makefile.am | 1 + common/utils/Makefile.am | 3 +- common/include/checked-overflow.h | 61 +++++++++++++++++++++++++++++++ common/utils/vector.c | 26 +++++++++---- 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/common/include/Makefile.am b/common/include/Makefile.am index a7d0d026..52d97216 100644 --- a/common/include/Makefile.am +++ b/common/include/Makefile.am @@ -37,6 +37,7 @@ EXTRA_DIST = \ ascii-ctype.h \ ascii-string.h \ byte-swapping.h \ + checked-overflow.h \ exit-with-parent.h \ isaligned.h \ ispowerof2.h \ diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am index 55415535..012a5c25 100644 --- a/common/utils/Makefile.am +++ b/common/utils/Makefile.am @@ -52,6 +52,7 @@ libutils_la_SOURCES = \ $(NULL) libutils_la_CPPFLAGS = \ -I$(top_srcdir)/include \ + -I$(top_srcdir)/common/include \ $(NULL) libutils_la_CFLAGS = \ $(WARNINGS_CFLAGS) \ @@ -101,7 +102,7 @@ test_quotes_CPPFLAGS = -I$(srcdir) test_quotes_CFLAGS = $(WARNINGS_CFLAGS) test_vector_SOURCES = test-vector.c vector.c vector.h bench.h -test_vector_CPPFLAGS = -I$(srcdir) +test_vector_CPPFLAGS = -I$(srcdir) -I$(top_srcdir)/common/include test_vector_CFLAGS = $(WARNINGS_CFLAGS) bench: test-vector diff --git a/common/include/checked-overflow.h b/common/include/checked-overflow.h new file mode 100644 index 00000000..b571e2c6 --- /dev/null +++ b/common/include/checked-overflow.h @@ -0,0 +1,61 @@ +/* nbdkit + * Copyright (C) 2013-2021 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +/* This header file defines functions for checking overflow in common + * integer arithmetic operations. + * + * It uses GCC/Clang built-ins: a possible future enhancement is to + * provide fallbacks in plain C or for other compilers. The only + * purpose of having a header file for this is to have a single place + * where we would extend this in future. + */ + +#ifndef NBDKIT_CHECKED_OVERFLOW_H +#define NBDKIT_CHECKED_OVERFLOW_H + +#if !defined(__GNUC__) && !defined(__clang__) +#error "this file may need to be ported to your compiler" +#endif + +/* Add two uint64_t values. Returns true if overflow happened. */ +#define ADD_UINT64_T_OVERFLOW(a, b, r) __builtin_add_overflow((a), (b), (r)) + +/* Multiply two uint64_t values. Returns true if overflow happened. */ +#define MUL_UINT64_T_OVERFLOW(a, b, r) __builtin_mul_overflow((a), (b), (r)) + +/* Add two size_t values. Returns true if overflow happened. */ +#define ADD_SIZE_T_OVERFLOW(a, b, r) __builtin_add_overflow((a), (b), (r)) + +/* Multiple two size_t values. Returns true if overflow happened. */ +#define MUL_SIZE_T_OVERFLOW(a, b, r) __builtin_mul_overflow((a), (b), (r)) + +#endif /* NBDKIT_CHECKED_OVERFLOW_H */ diff --git a/common/utils/vector.c b/common/utils/vector.c index dff051e9..e4ea7f3f 100644 --- a/common/utils/vector.c +++ b/common/utils/vector.c @@ -36,6 +36,7 @@ #include <stdlib.h> #include <errno.h> +#include "checked-overflow.h" #include "vector.h" int @@ -44,21 +45,30 @@ generic_vector_reserve (struct generic_vector *v, size_t n, size_t itemsize) void *newptr; size_t reqcap, reqbytes, newcap, newbytes; - /* New capacity requested. We must allocate this minimum (or fail). */ - reqcap = v->cap + n; - reqbytes = reqcap * itemsize; - if (reqbytes < v->cap * itemsize) { + /* New capacity requested. We must allocate this minimum (or fail). + * reqcap = v->cap + n + * reqbytes = reqcap * itemsize + */

+ if (ADD_SIZE_T_OVERFLOW (v->cap, n, &reqcap) || + MUL_SIZE_T_OVERFLOW (reqcap, itemsize, &reqbytes)) {

errno = ENOMEM; - return -1; /* overflow */ + return -1; } /* However for the sake of optimization, scale buffer by 3/2 so that * repeated reservations don't call realloc often. + * newcap = v->cap + (v->cap + 1) / 2 + * newbytes = newcap * itemsize */ - newcap = v->cap + (v->cap + 1) / 2; - newbytes = newcap * itemsize; - + if (ADD_SIZE_T_OVERFLOW (v->cap, 1, &newcap)) + goto fallback; + newcap /= 2; + if (ADD_SIZE_T_OVERFLOW (v->cap, newcap, &newcap)) + goto fallback;

+ if (MUL_SIZE_T_OVERFLOW (newcap, itemsize, &newbytes)) + goto fallback; if (newbytes < reqbytes) { + fallback: