Laszlo Ersek
2021-Nov-26 14:06 UTC
[Libguestfs] [nbdkit PATCH v3 0/3] common/include/checked-overflow: provide fallback
v2:
- https://listman.redhat.com/archives/libguestfs/2021-November/msg00249.html
- https://listman.redhat.com/archives/libguestfs/2021-November/msg00255.html
Please see the Notes section on each patch.
I built this version on RHEL7 as well, and I ran the test suite. I had
to forcibly disable two tests, test-tls and test-cache-max-size, for the
suite to complete. I didn't know initially if this was expected on
RHEL7, but then I peeked at the CentOS 7 SRPM, and the spec file in that
disables numerous tests, including TLS and cache related ones.
Thanks,
Laszlo
Laszlo Ersek (3):
common: rename UNIQUE_VAR to UNIQUE_NAME, and move it to its own
header
utils/vector: pass only unsigned arguments to (ADD|MUL)_OVERFLOW
common/include/checked-overflow: provide fallback
.gitignore | 1 +
common/include/Makefile.am | 6 +
common/include/checked-overflow.h | 167 +++++++++++++++++++++--
common/include/test-checked-overflow.c | 177 +++++++++++++++++++++++++
common/include/unique-name.h | 41 ++++++
common/utils/cleanup.h | 31 ++---
common/utils/vector.c | 2 +-
configure.ac | 6 +
filters/exitwhen/Makefile.am | 3 +-
filters/ext2/Makefile.am | 3 +-
filters/limit/Makefile.am | 3 +-
filters/log/Makefile.am | 3 +-
filters/offset/Makefile.am | 3 +-
filters/xz/Makefile.am | 3 +-
plugins/cc/Makefile.am | 1 +
plugins/cdi/Makefile.am | 1 +
plugins/iso/Makefile.am | 1 +
plugins/memory/Makefile.am | 3 +-
plugins/ondemand/Makefile.am | 3 +-
plugins/perl/Makefile.am | 3 +-
plugins/python/Makefile.am | 3 +-
plugins/split/Makefile.am | 3 +-
plugins/tmpdisk/Makefile.am | 3 +-
server/internal.h | 2 +-
tests/Makefile.am | 6 +-
25 files changed, 434 insertions(+), 44 deletions(-)
create mode 100644 common/include/unique-name.h
create mode 100644 common/include/test-checked-overflow.c
base-commit: b9a2c12da886decf662f4e2bbcd1699a200676aa
--
2.19.1.3.g30247aa5d201
Laszlo Ersek
2021-Nov-26 14:06 UTC
[Libguestfs] [nbdkit PATCH v3 1/3] common: rename UNIQUE_VAR to UNIQUE_NAME, and move it to its own header
I'd like to use UNIQUE_VAR() outside of "common/utils/cleanup.h";
also,
I'd like to use it for type names as well.
Give UNIQUE_VAR() its own header file, and rename it to UNIQUE_NAME().
A number of Makefiles (using the cleanup module) have to be refreshed;
while at it, make sure the "-I$(top_srcdir)/..." lists are
consistently
sorted, and terminated with $(NULL).
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
v3:
- new in v3
common/include/Makefile.am | 1 +
filters/exitwhen/Makefile.am | 3 ++-
filters/ext2/Makefile.am | 3 ++-
filters/limit/Makefile.am | 3 ++-
filters/log/Makefile.am | 3 ++-
filters/offset/Makefile.am | 3 ++-
filters/xz/Makefile.am | 3 ++-
plugins/cc/Makefile.am | 1 +
plugins/cdi/Makefile.am | 1 +
plugins/iso/Makefile.am | 1 +
plugins/memory/Makefile.am | 3 ++-
plugins/ondemand/Makefile.am | 3 ++-
plugins/perl/Makefile.am | 3 ++-
plugins/python/Makefile.am | 3 ++-
plugins/split/Makefile.am | 3 ++-
plugins/tmpdisk/Makefile.am | 3 ++-
tests/Makefile.am | 6 +++++-
common/include/unique-name.h | 41 ++++++++++++++++++++++++++++++++++++
common/utils/cleanup.h | 31 ++++++++++++---------------
server/internal.h | 2 +-
20 files changed, 89 insertions(+), 31 deletions(-)
create mode 100644 common/include/unique-name.h
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 52d97216fe2b..5421a5e2decf 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -47,6 +47,7 @@ EXTRA_DIST = \
random.h \
rounding.h \
tvdiff.h \
+ unique-name.h \
unix-path-max.h \
$(NULL)
diff --git a/filters/exitwhen/Makefile.am b/filters/exitwhen/Makefile.am
index 68e61d4bb9a8..19985ece7ff6 100644
--- a/filters/exitwhen/Makefile.am
+++ b/filters/exitwhen/Makefile.am
@@ -41,9 +41,10 @@ nbdkit_exitwhen_filter_la_SOURCES = \
$(NULL)
nbdkit_exitwhen_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/replacements \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_exitwhen_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_exitwhen_filter_la_LIBADD = \
diff --git a/filters/ext2/Makefile.am b/filters/ext2/Makefile.am
index c73f963d0508..2266a7fdbf34 100644
--- a/filters/ext2/Makefile.am
+++ b/filters/ext2/Makefile.am
@@ -45,8 +45,9 @@ nbdkit_ext2_filter_la_SOURCES = \
$(NULL)
nbdkit_ext2_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_ext2_filter_la_CFLAGS = \
$(WARNINGS_CFLAGS) \
diff --git a/filters/limit/Makefile.am b/filters/limit/Makefile.am
index b4951acef5ff..bea3d9f18844 100644
--- a/filters/limit/Makefile.am
+++ b/filters/limit/Makefile.am
@@ -41,8 +41,9 @@ nbdkit_limit_filter_la_SOURCES = \
$(NULL)
nbdkit_limit_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_limit_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_limit_filter_la_LDFLAGS = \
diff --git a/filters/log/Makefile.am b/filters/log/Makefile.am
index 8b0d9facc28d..26241051fd6d 100644
--- a/filters/log/Makefile.am
+++ b/filters/log/Makefile.am
@@ -46,8 +46,9 @@ nbdkit_log_filter_la_SOURCES = \
$(NULL)
nbdkit_log_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_log_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_log_filter_la_LDFLAGS = \
diff --git a/filters/offset/Makefile.am b/filters/offset/Makefile.am
index 5437e700177e..a596b6514cda 100644
--- a/filters/offset/Makefile.am
+++ b/filters/offset/Makefile.am
@@ -41,8 +41,9 @@ nbdkit_offset_filter_la_SOURCES = \
$(NULL)
nbdkit_offset_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_offset_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_offset_filter_la_LDFLAGS = \
diff --git a/filters/xz/Makefile.am b/filters/xz/Makefile.am
index ce6bc3423bee..742991db1a1d 100644
--- a/filters/xz/Makefile.am
+++ b/filters/xz/Makefile.am
@@ -47,8 +47,9 @@ nbdkit_xz_filter_la_SOURCES = \
$(NULL)
nbdkit_xz_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_xz_filter_la_CFLAGS = \
$(WARNINGS_CFLAGS) \
diff --git a/plugins/cc/Makefile.am b/plugins/cc/Makefile.am
index ffee90c5eb9f..ea36adcacd4e 100644
--- a/plugins/cc/Makefile.am
+++ b/plugins/cc/Makefile.am
@@ -47,6 +47,7 @@ nbdkit_cc_plugin_la_SOURCES = \
nbdkit_cc_plugin_la_CPPFLAGS = \
-DCC="\"$(CC)\"" \
-DCFLAGS="\"$(CFLAGS) -fPIC -shared\"" \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
-I$(top_srcdir)/include \
-I. \
diff --git a/plugins/cdi/Makefile.am b/plugins/cdi/Makefile.am
index be042ba1fc99..7a31da3dfa86 100644
--- a/plugins/cdi/Makefile.am
+++ b/plugins/cdi/Makefile.am
@@ -44,6 +44,7 @@ nbdkit_cdi_plugin_la_SOURCES = \
$(NULL)
nbdkit_cdi_plugin_la_CPPFLAGS = \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
-I$(top_srcdir)/include \
-I. \
diff --git a/plugins/iso/Makefile.am b/plugins/iso/Makefile.am
index f419feb2c5ad..abd9595b4202 100644
--- a/plugins/iso/Makefile.am
+++ b/plugins/iso/Makefile.am
@@ -46,6 +46,7 @@ nbdkit_iso_plugin_la_SOURCES = \
$(NULL)
nbdkit_iso_plugin_la_CPPFLAGS = \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
-I$(top_srcdir)/include \
-I. \
diff --git a/plugins/memory/Makefile.am b/plugins/memory/Makefile.am
index ab0f3a0dcac4..428914eff48f 100644
--- a/plugins/memory/Makefile.am
+++ b/plugins/memory/Makefile.am
@@ -41,10 +41,11 @@ nbdkit_memory_plugin_la_SOURCES = \
$(NULL)
nbdkit_memory_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
-I$(top_srcdir)/common/allocators \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/replacements \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_memory_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_memory_plugin_la_LDFLAGS = \
diff --git a/plugins/ondemand/Makefile.am b/plugins/ondemand/Makefile.am
index 3c391416ee0c..e5be80706d49 100644
--- a/plugins/ondemand/Makefile.am
+++ b/plugins/ondemand/Makefile.am
@@ -60,9 +60,10 @@ nbdkit_ondemand_plugin_la_SOURCES = \
$(NULL)
nbdkit_ondemand_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/replacements \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_ondemand_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_ondemand_plugin_la_LDFLAGS = \
diff --git a/plugins/perl/Makefile.am b/plugins/perl/Makefile.am
index 89fe733e8261..a5b654a04f0d 100644
--- a/plugins/perl/Makefile.am
+++ b/plugins/perl/Makefile.am
@@ -46,8 +46,9 @@ nbdkit_perl_plugin_la_SOURCES = \
$(NULL)
nbdkit_perl_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_perl_plugin_la_CFLAGS = \
$(WARNINGS_CFLAGS) \
diff --git a/plugins/python/Makefile.am b/plugins/python/Makefile.am
index 2d8955ef566f..e2cd6116aacc 100644
--- a/plugins/python/Makefile.am
+++ b/plugins/python/Makefile.am
@@ -53,8 +53,9 @@ nbdkit_python_plugin_la_SOURCES = \
$(NULL)
nbdkit_python_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_python_plugin_la_CFLAGS = \
diff --git a/plugins/split/Makefile.am b/plugins/split/Makefile.am
index 1ac0fefef6fc..ffd69d19c733 100644
--- a/plugins/split/Makefile.am
+++ b/plugins/split/Makefile.am
@@ -41,9 +41,10 @@ nbdkit_split_plugin_la_SOURCES = \
$(NULL)
nbdkit_split_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/replacements \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_split_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_split_plugin_la_LDFLAGS = \
diff --git a/plugins/tmpdisk/Makefile.am b/plugins/tmpdisk/Makefile.am
index ae263e1fe0d6..03c1af458fb7 100644
--- a/plugins/tmpdisk/Makefile.am
+++ b/plugins/tmpdisk/Makefile.am
@@ -59,8 +59,9 @@ nbdkit_tmpdisk_plugin_la_SOURCES = \
$(NULL)
nbdkit_tmpdisk_plugin_la_CPPFLAGS = \
- -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
+ -I$(top_srcdir)/include \
$(NULL)
nbdkit_tmpdisk_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
nbdkit_tmpdisk_plugin_la_LIBADD = \
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 64b56efbba92..9371c62b074a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -603,6 +603,7 @@ test_curl_header_script_SOURCES = \
web-server.h \
$(NULL)
test_curl_header_script_CPPFLAGS = \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
$(NULL)
test_curl_header_script_CFLAGS = \
@@ -624,6 +625,7 @@ test_curl_cookie_script_SOURCES = \
web-server.h \
$(NULL)
test_curl_cookie_script_CPPFLAGS = \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
$(NULL)
test_curl_cookie_script_CFLAGS = \
@@ -1724,7 +1726,9 @@ test_retry_request_mirror_SOURCES = \
test.h \
$(NULL)
test_retry_request_mirror_CPPFLAGS = \
- -I$(top_srcdir)/common/utils
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils \
+ $(NULL)
test_retry_request_mirror_CFLAGS = \
$(WARNINGS_CFLAGS) \
$(PTHREAD_CFLAGS) \
diff --git a/common/include/unique-name.h b/common/include/unique-name.h
new file mode 100644
index 000000000000..31d937fdbbad
--- /dev/null
+++ b/common/include/unique-name.h
@@ -0,0 +1,41 @@
+/* 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.
+ */
+
+#ifndef NBDKIT_UNIQUE_NAME_H
+#define NBDKIT_UNIQUE_NAME_H
+
+/* https://stackoverflow.com/a/1597129 */
+#define XXUNIQUE_NAME(name, line) name ## line
+#define XUNIQUE_NAME(name, line) XXUNIQUE_NAME (name, line)
+#define UNIQUE_NAME(name) XUNIQUE_NAME (name, __LINE__)
+
+#endif /* NBDKIT_UNIQUE_NAME_H */
diff --git a/common/utils/cleanup.h b/common/utils/cleanup.h
index e6f899663663..ced16a009495 100644
--- a/common/utils/cleanup.h
+++ b/common/utils/cleanup.h
@@ -36,6 +36,8 @@
#include <pthread.h>
#include <assert.h>
+#include "unique-name.h"
+
/* Work around clang bug: https://bugs.llvm.org/show_bug.cgi?id=43482 */
#ifdef __clang__
#define CLANG_UNUSED_VARIABLE_WORKAROUND __attribute__((__unused__))
@@ -43,11 +45,6 @@
#define CLANG_UNUSED_VARIABLE_WORKAROUND
#endif
-/* https://stackoverflow.com/a/1597129 */
-#define XXUNIQUE_VAR(name, line) name ## line
-#define XUNIQUE_VAR(name, line) XXUNIQUE_VAR (name, line)
-#define UNIQUE_VAR(name) XUNIQUE_VAR (name, __LINE__)
-
/* cleanup.c */
extern void cleanup_free (void *ptr);
#define CLEANUP_FREE __attribute__((cleanup (cleanup_free)))
@@ -56,27 +53,27 @@ extern void cleanup_mutex_unlock (pthread_mutex_t **ptr);
#define CLEANUP_MUTEX_UNLOCK __attribute__((cleanup (cleanup_mutex_unlock)))
#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \
- CLEANUP_MUTEX_UNLOCK pthread_mutex_t *UNIQUE_VAR(_lock) = mutex; \
+ CLEANUP_MUTEX_UNLOCK pthread_mutex_t *UNIQUE_NAME(_lock) = mutex; \
do { \
- int _r = pthread_mutex_lock (UNIQUE_VAR(_lock)); \
+ int _r = pthread_mutex_lock (UNIQUE_NAME(_lock)); \
assert (!_r); \
} while (0)
extern void cleanup_rwlock_unlock (pthread_rwlock_t **ptr);
#define CLEANUP_RWLOCK_UNLOCK __attribute__((cleanup (cleanup_rwlock_unlock)))
-#define ACQUIRE_WRLOCK_FOR_CURRENT_SCOPE(rwlock) \
- CLEANUP_RWLOCK_UNLOCK pthread_rwlock_t *UNIQUE_VAR(_rwlock) = rwlock; \
- do { \
- int _r = pthread_rwlock_wrlock (UNIQUE_VAR(_rwlock)); \
- assert (!_r); \
+#define ACQUIRE_WRLOCK_FOR_CURRENT_SCOPE(rwlock) \
+ CLEANUP_RWLOCK_UNLOCK pthread_rwlock_t *UNIQUE_NAME(_rwlock) = rwlock; \
+ do { \
+ int _r = pthread_rwlock_wrlock (UNIQUE_NAME(_rwlock)); \
+ assert (!_r); \
} while (0)
-#define ACQUIRE_RDLOCK_FOR_CURRENT_SCOPE(rwlock) \
- CLEANUP_RWLOCK_UNLOCK pthread_rwlock_t *UNIQUE_VAR(_rwlock) = rwlock; \
- do { \
- int _r = pthread_rwlock_rdlock (UNIQUE_VAR(_rwlock)); \
- assert (!_r); \
+#define ACQUIRE_RDLOCK_FOR_CURRENT_SCOPE(rwlock) \
+ CLEANUP_RWLOCK_UNLOCK pthread_rwlock_t *UNIQUE_NAME(_rwlock) = rwlock; \
+ do { \
+ int _r = pthread_rwlock_rdlock (UNIQUE_NAME(_rwlock)); \
+ assert (!_r); \
} while (0)
/* cleanup-nbdkit.c */
diff --git a/server/internal.h b/server/internal.h
index bc81b7861848..ccbf0f062d28 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -531,7 +531,7 @@ extern void threadlocal_pop_context (struct context **ctx);
#define CLEANUP_CONTEXT_POP __attribute__((cleanup (threadlocal_pop_context)))
#define PUSH_CONTEXT_FOR_SCOPE(ctx) \
CLEANUP_CONTEXT_POP CLANG_UNUSED_VARIABLE_WORKAROUND \
- struct context *UNIQUE_VAR(_ctx) = threadlocal_push_context (ctx)
+ struct context *UNIQUE_NAME(_ctx) = threadlocal_push_context (ctx)
/* Macro which sets local variable struct connection *conn from
* thread-local storage, asserting that it is non-NULL. If you want
--
2.19.1.3.g30247aa5d201
Laszlo Ersek
2021-Nov-26 14:06 UTC
[Libguestfs] [nbdkit PATCH v3 2/3] utils/vector: pass only unsigned arguments to (ADD|MUL)_OVERFLOW
In a subsequent patch, the (ADD|MUL)_OVERFLOW macros will require each
operand to have some unsigned integer type. To prevent a build failure in
"common/utils/vector.c", satisfy that requirement in advance.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
common/utils/vector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/utils/vector.c b/common/utils/vector.c
index ee7591560ca5..54e6b810bea6 100644
--- a/common/utils/vector.c
+++ b/common/utils/vector.c
@@ -60,7 +60,7 @@ generic_vector_reserve (struct generic_vector *v, size_t n,
size_t itemsize)
* newcap = v->cap + (v->cap + 1) / 2
* newbytes = newcap * itemsize
*/
- if (ADD_OVERFLOW (v->cap, 1, &t) ||
+ if (ADD_OVERFLOW (v->cap, 1u, &t) ||
ADD_OVERFLOW (v->cap, t/2, &newcap) ||
MUL_OVERFLOW (newcap, itemsize, &newbytes) ||
newbytes < reqbytes) {
--
2.19.1.3.g30247aa5d201
Laszlo Ersek
2021-Nov-26 14:06 UTC
[Libguestfs] [nbdkit PATCH v3 3/3] common/include/checked-overflow: provide fallback
On RHEL 7 (GCC 4.8.5) we don't have __builtin_add_overflow and similar
functions. They were first added in GCC 5. Provide a fallback path for
these older compilers.
The minimum GCC version we want to support (for the sake of FreeBSD) is
4.2. The fallback path uses the "typeof" and "statement
expression" GCC
extensions; those are available in gcc-4.2:
- https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Statement-Exprs.html
- https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Typeof.html
The math and the macros were discussed in the following thread:
- [Libguestfs] [PATCH nbdkit 2/2] common/include/checked-overflow.h:
Provide fallback
Message-Id: <20211115121417.1174272-2-rjones at redhat.com>
https://listman.redhat.com/archives/libguestfs/2021-November/msg00177.html
Part of the commit message, and the "configure.ac" hunk, were stolen
from
Rich's original patch, linked above.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
v3:
- use parens around '&&' within '||', in
check_mul_overflow() [Rich /
RHEL7 gcc-4.8.5]
- mark the "x_has_uint_type" type as "unused" [Rich /
RHEL7 gcc-4.8.5]
- move test-checked-overflow to common/include [Rich]
- while at it, refresh _SOURCES (depend on "checked-overflow.h"
too)
and _CPPFLAGS (can be just "-I$(srcdir)", like with the other
test
programs in the same Makefile) [Laszlo]
- add test program to .gitignore [Rich]
- Prevent shadowing existent variables and type names by using
UNIQUE_NAME in the block scope declarations / definitions of:
ADD_OVERFLOW_FALLBACK, MUL_OVERFLOW_FALLBACK,
STATIC_ASSERT_UNSIGNED_INT, TEST_ADD, TEST_MUL.
Adopt the "leading underscore followed by lowercase letter"
pattern
for the names, from existing uses of UNIQUE_NAME.
v2:
> diff --git a/common/utils/test-checked-overflow.c
b/common/utils/test-checked-overflow.c
> index 2158950cff22..c2882e38b094 100644
> --- a/common/utils/test-checked-overflow.c
> +++ b/common/utils/test-checked-overflow.c
> @@ -153,11 +153,11 @@ main (void)
> */
> overflow = MUL_OVERFLOW_FALLBACK (3u, 5u, &result.u8);
> assert (!overflow);
> - assert (result.u8 = 0xF);
> + assert (result.u8 == 0xF);
>
> overflow = MUL_OVERFLOW_FALLBACK (result.u8, 17u, &result.u8);
> assert (!overflow);
> - assert (result.u8 = UINT8_MAX);
> + assert (result.u8 == UINT8_MAX);
>
> overflow = MUL_OVERFLOW_FALLBACK (result.u8, 257u, &result.u16);
> assert (!overflow);
configure.ac | 6 +
common/include/Makefile.am | 5 +
common/include/checked-overflow.h | 167 +++++++++++++++++++++--
common/include/test-checked-overflow.c | 177 +++++++++++++++++++++++++
.gitignore | 1 +
5 files changed, 344 insertions(+), 12 deletions(-)
create mode 100644 common/include/test-checked-overflow.c
diff --git a/configure.ac b/configure.ac
index 6092b6cc957f..e22fd01f535f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -220,6 +220,12 @@ CFLAGS="$old_CFLAGS"
AC_MSG_RESULT([$supports_std_c90])
AM_CONDITIONAL([CAN_TEST_ANSI_C], [test "x$supports_std_c90" =
"xyes"])
+dnl Check for __builtin_*_overflow. We need the dummy parameters
+dnl else detection doesn't work correctly for some reason.
+AC_CHECK_DECLS([__builtin_add_overflow(int, int, int *),
+ __builtin_mul_overflow(int, int, int *)],
+ [], [], [])
+
dnl On Haiku we must use BSD-compatibility headers to get the endian
dnl macros we use.
AC_MSG_CHECKING(whether OS-dependent include paths are required)
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 5421a5e2decf..a8d9617c2432 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -57,6 +57,7 @@ TESTS = \
test-ascii-ctype \
test-ascii-string \
test-byte-swapping \
+ test-checked-overflow \
test-isaligned \
test-ispowerof2 \
test-iszero \
@@ -79,6 +80,10 @@ test_byte_swapping_SOURCES = test-byte-swapping.c
byte-swapping.h
test_byte_swapping_CPPFLAGS = -I$(srcdir)
test_byte_swapping_CFLAGS = $(WARNINGS_CFLAGS)
+test_checked_overflow_SOURCES = test-checked-overflow.c checked-overflow.h
+test_checked_overflow_CPPFLAGS = -I$(srcdir)
+test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
+
test_isaligned_SOURCES = test-isaligned.c isaligned.h
test_isaligned_CPPFLAGS = -I$(srcdir)
test_isaligned_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/checked-overflow.h
b/common/include/checked-overflow.h
index ddc4b487b488..2e46c1c92f75 100644
--- a/common/include/checked-overflow.h
+++ b/common/include/checked-overflow.h
@@ -30,13 +30,17 @@
* SUCH DAMAGE.
*/
-/* This header file defines functions for checking overflow in common
- * integer arithmetic operations.
+/* This header file defines macros and 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.
+ * The macros use:
+ * - the "statement expression" GCC extension,
+ * - the "typeof" GCC extension,
+ * - the __builtin_add_overflow() and __builtin_mul_overflow() GCC/Clang
+ * built-ins.
+ *
+ * If either built-in is unavailable, the corresponding macro replaces it with
+ * a call to an inline C function.
*/
#ifndef NBDKIT_CHECKED_OVERFLOW_H
@@ -46,14 +50,153 @@
#error "this file may need to be ported to your compiler"
#endif
-/* Add two values. *r = a + b
- * Returns true if overflow happened.
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "unique-name.h"
+
+/* Add "a" and "b", both of (possibly different) unsigned
integer types, and
+ * store the sum in "*r", which must also have some unsigned integer
type.
+ *
+ * Each macro argument is evaluated exactly once, as long as it does not have
+ * variably modified type.
+ *
+ * The macro evaluates to "false" if "*r" can represent the
mathematical sum.
+ * Otherwise, the macro evaluates to "true", and the low order bits
of the
+ * mathematical sum are stored to "*r".
+ */
+#if HAVE_DECL___BUILTIN_ADD_OVERFLOW
+#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_BUILTIN((a), (b), (r))
+#else
+#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_FALLBACK((a), (b), (r))
+#endif
+
+/* Multiply "a" and "b", both of (possibly different)
unsigned integer types,
+ * and store the product in "*r", which must also have some unsigned
integer
+ * type.
+ *
+ * Each macro argument is evaluated exactly once, as long as it does not have
+ * variably modified type.
+ *
+ * The macro evaluates to "false" if "*r" can represent the
mathematical
+ * product. Otherwise, the macro evaluates to "true", and the low
order bits of
+ * the mathematical product are stored to "*r".
+ */
+#if HAVE_DECL___BUILTIN_MUL_OVERFLOW
+#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_BUILTIN((a), (b), (r))
+#else
+#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_FALLBACK((a), (b), (r))
+#endif
+
+/* The ADD_OVERFLOW_BUILTIN and MUL_OVERFLOW_BUILTIN function-like macros
+ * enforce the unsignedness of all their operands even though the underlying
+ * compiler built-ins, __builtin_add_overflow() and __builtin_mul_overflow(),
+ * don't depend on that. The explanation is that the fallback
implementation
+ * does depend on the unsignedness of all operands, and the codebase should
+ * seamlessly build regardless of the built-in vs. fallback choice.
+ *
+ * Each macro argument is evaluated exactly once, as long as it does not have
+ * variably modified type.
+ */
+#if HAVE_DECL___BUILTIN_ADD_OVERFLOW
+#define ADD_OVERFLOW_BUILTIN(a, b, r) \
+ ({ \
+ STATIC_ASSERT_UNSIGNED_INT (a); \
+ STATIC_ASSERT_UNSIGNED_INT (b); \
+ STATIC_ASSERT_UNSIGNED_INT (*(r)); \
+ __builtin_add_overflow((a), (b), (r)); \
+ })
+#endif
+
+#if HAVE_DECL___BUILTIN_MUL_OVERFLOW
+#define MUL_OVERFLOW_BUILTIN(a, b, r) \
+ ({ \
+ STATIC_ASSERT_UNSIGNED_INT (a); \
+ STATIC_ASSERT_UNSIGNED_INT (b); \
+ STATIC_ASSERT_UNSIGNED_INT (*(r)); \
+ __builtin_mul_overflow((a), (b), (r)); \
+ })
+#endif
+
+/* The fallback macros call inline C functions. The unsignedness of all
+ * operands is enforced in order to keep the conversion to uintmax_t
+ * value-preserving, and to keep the conversion back from uintmax_t independent
+ * of the C language implementation.
+ *
+ * Each macro argument is evaluated exactly once, as long as it does not have
+ * variably modified type.
+ *
+ * The fallback macros and the inline C functions are defined regardless of
+ * HAVE_DECL___BUILTIN_ADD_OVERFLOW and HAVE_DECL___BUILTIN_MUL_OVERFLOW so
+ * that the test suite can always test the fallback.
+ */
+#define ADD_OVERFLOW_FALLBACK(a, b, r) \
+ ({ \
+ bool UNIQUE_NAME(_overflow); \
+ uintmax_t UNIQUE_NAME(_tmp); \
+ \
+ STATIC_ASSERT_UNSIGNED_INT (a); \
+ STATIC_ASSERT_UNSIGNED_INT (b); \
+ STATIC_ASSERT_UNSIGNED_INT (*(r)); \
+ UNIQUE_NAME(_overflow) = check_add_overflow ((a), (b), \
+ (typeof (*(r)))-1, \
+ &UNIQUE_NAME(_tmp)); \
+ *(r) = UNIQUE_NAME(_tmp); \
+ UNIQUE_NAME(_overflow); \
+ })
+
+#define MUL_OVERFLOW_FALLBACK(a, b, r) \
+ ({ \
+ bool UNIQUE_NAME(_overflow); \
+ uintmax_t UNIQUE_NAME(_tmp); \
+ \
+ STATIC_ASSERT_UNSIGNED_INT (a); \
+ STATIC_ASSERT_UNSIGNED_INT (b); \
+ STATIC_ASSERT_UNSIGNED_INT (*(r)); \
+ UNIQUE_NAME(_overflow) = check_mul_overflow ((a), (b), \
+ (typeof (*(r)))-1, \
+ &UNIQUE_NAME(_tmp)); \
+ *(r) = UNIQUE_NAME(_tmp); \
+ UNIQUE_NAME(_overflow); \
+ })
+
+/* Assert, at compile time, that the expression "x" has some unsigned
integer
+ * type.
+ *
+ * The expression "x" is not evaluated, unless it has variably
modified type.
*/
-#define ADD_OVERFLOW(a, b, r) __builtin_add_overflow((a), (b), (r))
+#define STATIC_ASSERT_UNSIGNED_INT(x) \
+ do { \
+ typedef char UNIQUE_NAME(_x_has_uint_type)[(typeof (x))-1 > 0 ? 1 : -1]
\
+ __attribute__((__unused__)); \
+ } while (0)
-/* Multiply two values. *r = a * b
- * Returns true if overflow happened.
+/* Assign the sum "a + b" to "*r", using uintmax_t modular
arithmetic.
+ *
+ * Return true iff the addition overflows or the result exceeds
"max".
*/
-#define MUL_OVERFLOW(a, b, r) __builtin_mul_overflow((a), (b), (r))
+static inline bool
+check_add_overflow (uintmax_t a, uintmax_t b, uintmax_t max, uintmax_t *r)
+{
+ bool in_range;
+
+ *r = a + b;
+ in_range = a <= UINTMAX_MAX - b && *r <= max;
+ return !in_range;
+}
+
+/* Assign the product "a * b" to "*r", using uintmax_t
modular arithmetic.
+ *
+ * Return true iff the multiplication overflows or the result exceeds
"max".
+ */
+static inline bool
+check_mul_overflow (uintmax_t a, uintmax_t b, uintmax_t max, uintmax_t *r)
+{
+ bool in_range;
+
+ *r = a * b;
+ in_range = b == 0 || (a <= UINTMAX_MAX / b && *r <= max);
+ return !in_range;
+}
#endif /* NBDKIT_CHECKED_OVERFLOW_H */
diff --git a/common/include/test-checked-overflow.c
b/common/include/test-checked-overflow.c
new file mode 100644
index 000000000000..76d479d8073e
--- /dev/null
+++ b/common/include/test-checked-overflow.c
@@ -0,0 +1,177 @@
+/* nbdkit
+ * Copyright (C) 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.
+ */
+
+#include <stddef.h>
+#include <stdint.h>
+#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */
+#include <assert.h>
+
+#include "checked-overflow.h"
+
+#define TEST_ADD(a, b, result, expected_overflow, expected_result) \
+ do { \
+ bool UNIQUE_NAME(_overflow); \
+ \
+ UNIQUE_NAME(_overflow) = ADD_OVERFLOW_FALLBACK ((a), (b), (result)); \
+ assert (UNIQUE_NAME(_overflow) == (expected_overflow)); \
+ assert (*(result) == (expected_result)); \
+ UNIQUE_NAME(_overflow) = ADD_OVERFLOW_FALLBACK ((b), (a), (result)); \
+ assert (UNIQUE_NAME(_overflow) == (expected_overflow)); \
+ assert (*(result) == (expected_result)); \
+ } while (0)
+
+#define TEST_MUL(a, b, result, expected_overflow, expected_result) \
+ do { \
+ bool UNIQUE_NAME(_overflow); \
+ \
+ UNIQUE_NAME(_overflow) = MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \
+ assert (UNIQUE_NAME(_overflow) == (expected_overflow)); \
+ assert (*(result) == (expected_result)); \
+ UNIQUE_NAME(_overflow) = MUL_OVERFLOW_FALLBACK ((b), (a), (result)); \
+ assert (UNIQUE_NAME(_overflow) == (expected_overflow)); \
+ assert (*(result) == (expected_result)); \
+ } while (0)
+
+/* Define these const-qualified objects because the UINTN_MAX object-like
+ * macros in <stdint.h> have "post integer promotion" types.
Therefore,
+ * UINT16_MAX and UINT8_MAX most commonly have type "int". And that
trips the
+ * signedness check in ADD_OVERFLOW_FALLBACK().
+ */
+static const uintmax_t umax_max = UINTMAX_MAX;
+static const uint64_t u64_max = UINT64_MAX;
+static const uint32_t u32_max = UINT32_MAX;
+static const uint16_t u16_max = UINT16_MAX;
+static const uint8_t u8_max = UINT8_MAX;
+static const size_t size_max = SIZE_MAX;
+
+int
+main (void)
+{
+ union {
+ uintmax_t umax;
+ uint64_t u64;
+ uint32_t u32;
+ uint16_t u16;
+ uint8_t u8;
+ size_t sz;
+ } result;
+ bool overflow;
+
+ /* "max + 0" and "0 + max" evaluate to "max",
without overflow. */
+ TEST_ADD (umax_max, 0u, &result.umax, false, umax_max);
+ TEST_ADD (u64_max, 0u, &result.u64, false, u64_max);
+ TEST_ADD (u32_max, 0u, &result.u32, false, u32_max);
+ TEST_ADD (u16_max, 0u, &result.u16, false, u16_max);
+ TEST_ADD (u8_max, 0u, &result.u8, false, u8_max);
+ TEST_ADD (size_max, 0u, &result.sz, false, size_max);
+
+ /* "max + 1" and "1 + max" overflow to zero. */
+ TEST_ADD (umax_max, 1u, &result.umax, true, 0);
+ TEST_ADD (u64_max, 1u, &result.u64, true, 0);
+ TEST_ADD (u32_max, 1u, &result.u32, true, 0);
+ TEST_ADD (u16_max, 1u, &result.u16, true, 0);
+ TEST_ADD (u8_max, 1u, &result.u8, true, 0);
+ TEST_ADD (size_max, 1u, &result.sz, true, 0);
+
+ /* Adding umax_max (i.e., all-bits-one) amounts (with overflow) to
+ * subtracting one.
+ */
+ TEST_ADD (umax_max, umax_max, &result.umax, true, umax_max - 1);
+ TEST_ADD (u64_max, umax_max, &result.u64, true, u64_max - 1);
+ TEST_ADD (u32_max, umax_max, &result.u32, true, u32_max - 1);
+ TEST_ADD (u16_max, umax_max, &result.u16, true, u16_max - 1);
+ TEST_ADD (u8_max, umax_max, &result.u8, true, u8_max - 1);
+ TEST_ADD (size_max, umax_max, &result.sz, true, size_max - 1);
+
+ /* "max * 0" and "0 * max" evaluate to 0, without
overflow. */
+ TEST_MUL (umax_max, 0u, &result.umax, false, 0);
+ TEST_MUL (u64_max, 0u, &result.u64, false, 0);
+ TEST_MUL (u32_max, 0u, &result.u32, false, 0);
+ TEST_MUL (u16_max, 0u, &result.u16, false, 0);
+ TEST_MUL (u8_max, 0u, &result.u8, false, 0);
+ TEST_MUL (size_max, 0u, &result.sz, false, 0);
+
+ /* "max * 1" and "1 * max" evaluate to "max",
without overflow. */
+ TEST_MUL (umax_max, 1u, &result.umax, false, umax_max);
+ TEST_MUL (u64_max, 1u, &result.u64, false, u64_max);
+ TEST_MUL (u32_max, 1u, &result.u32, false, u32_max);
+ TEST_MUL (u16_max, 1u, &result.u16, false, u16_max);
+ TEST_MUL (u8_max, 1u, &result.u8, false, u8_max);
+ TEST_MUL (size_max, 1u, &result.sz, false, size_max);
+
+ /* "max * 2" and "2 * max" evaluate (with overflow) to
"max - 1". */
+ TEST_MUL (umax_max, 2u, &result.umax, true, umax_max - 1);
+ TEST_MUL (u64_max, 2u, &result.u64, true, u64_max - 1);
+ TEST_MUL (u32_max, 2u, &result.u32, true, u32_max - 1);
+ TEST_MUL (u16_max, 2u, &result.u16, true, u16_max - 1);
+ TEST_MUL (u8_max, 2u, &result.u8, true, u8_max - 1);
+ TEST_MUL (size_max, 2u, &result.sz, true, size_max - 1);
+
+ /* factor 255 -> 3 5 17
+ * factor 65535 -> 3 5 17 257
+ * factor 4294967295 -> 3 5 17 257 65537
+ * factor 18446744073709551615 -> 3 5 17 257 641 65537 6700417
+ *
+ * Note: every time we double the width, we multiply the previous maximum
+ * 0xF...F with 0x10...01:
+ *
+ * 0xF (= 3 * 5) * 0x11 (= 17) = 0xFF
+ * 0xFF * 0x101 (= 257) = 0xFFFF
+ * 0xFFFF * 0x10001 (= 65537) = 0xFFFFFFFF
+ * 0xFFFFFFFF * 0x100000001 (= 641 * 6700417) = 0xFFFFFFFFFFFFFFFF
+ *
+ * Perform the above multiplications, advacing with prime factors.
+ */
+ overflow = MUL_OVERFLOW_FALLBACK (3u, 5u, &result.u8);
+ assert (!overflow);
+ assert (result.u8 == 0xF);
+
+ overflow = MUL_OVERFLOW_FALLBACK (result.u8, 17u, &result.u8);
+ assert (!overflow);
+ assert (result.u8 == UINT8_MAX);
+
+ overflow = MUL_OVERFLOW_FALLBACK (result.u8, 257u, &result.u16);
+ assert (!overflow);
+ assert (result.u16 == UINT16_MAX);
+
+ overflow = MUL_OVERFLOW_FALLBACK (result.u16, 65537ul, &result.u32);
+ assert (!overflow);
+ assert (result.u32 == UINT32_MAX);
+
+ overflow = MUL_OVERFLOW_FALLBACK (result.u32, 641u, &result.u64);
+ assert (!overflow);
+ overflow = MUL_OVERFLOW_FALLBACK (result.u64, 6700417ul, &result.u64);
+ assert (!overflow);
+ assert (result.u64 == UINT64_MAX);
+
+ return 0;
+}
diff --git a/.gitignore b/.gitignore
index 4e2ae75d63f0..2d27cf06d52d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -37,6 +37,7 @@ plugins/*/*.3
/common/include/test-ascii-ctype
/common/include/test-ascii-string
/common/include/test-byte-swapping
+/common/include/test-checked-overflow
/common/include/test-isaligned
/common/include/test-ispowerof2
/common/include/test-iszero
--
2.19.1.3.g30247aa5d201