Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Ever since of v6.9.0-rc1~384 we allow zero sized bitmaps. If that's the case then the map is kept unallocated (NULL). Unfortunately, some virBitmap APIs call memcpy()/memset() which doesn't allow NULL pointers. Introduce checks to these APIs (virBitmapNewCopy(), virBitmapSetAll() and virBitmapClearAll()). While at it, also introduce a test case to virbitmaptest to catch UB in our CI.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbitmap.c | 9 ++++++++- tests/virbitmaptest.c | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 78545501b7..b9b737a087 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -587,7 +587,8 @@ virBitmapNewCopy(virBitmap *src) { virBitmap *dst = virBitmapNew(src->nbits);
- memcpy(dst->map, src->map, dst->map_len * sizeof(src->map[0])); + if (G_LIKELY(dst->map_len > 0)) + memcpy(dst->map, src->map, dst->map_len * sizeof(src->map[0]));
return dst; } @@ -770,6 +771,9 @@ virBitmapClearTail(virBitmap *bitmap) */ void virBitmapSetAll(virBitmap *bitmap) { + if (G_UNLIKELY(bitmap->map_len == 0)) + return; + memset(bitmap->map, 0xff, bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT));
@@ -786,6 +790,9 @@ void virBitmapSetAll(virBitmap *bitmap) void virBitmapClearAll(virBitmap *bitmap) { + if (G_UNLIKELY(bitmap->map_len == 0)) + return; + memset(bitmap->map, 0, bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT)); } diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 709c62ab54..549956ad04 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -768,6 +768,30 @@ test17(const void *opaque G_GNUC_UNUSED) }
+static int +test18(const void *opaque G_GNUC_UNUSED) +{ + g_autoptr(virBitmap) map = virBitmapNew(0); + g_autoptr(virBitmap) copy = virBitmapNewCopy(map); + + /* Creating an empty bitmap means underlying array of bits is + * unallocated (NULL). For some virBitmap APIs this may lead + * to UB. The whole point of this test is to call those and + * rely on clang's UB sanitizer to catch such errors at + * runtime. */ + + virBitmapSetAll(map); + virBitmapClearAll(copy); + + if (!virBitmapEqual(map, copy)) { + fprintf(stderr, "empty maps don't equal\n"); + return -1; + } + + return 0; +} + + #define TESTBINARYOP(A, B, RES, FUNC) \ testBinaryOpData.a = A; \ testBinaryOpData.b = B; \ @@ -847,6 +871,9 @@ mymain(void) if (virTestRun("test17", test17, NULL) < 0) ret = -1;
+ if (virTestRun("test18", test18, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
-- 2.55.0
Reviewed-by: Roman Bogorodskiy <bogorodskiy@gmail.com>