Re: [PATCH] virbitmap: Avoid UB with zero sized bitmaps

[Roman Bogorodskiy]

  Michal Privoznik via Devel wrote:

> From: Michal Privoznik <mpriv...@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 <mpriv...@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 <bogorods...@gmail.com>