Libguestfs - Sep 2019 - [PATCH nbdkit] common/bitmap: Don't fail on realloc (ptr, 0)

The following commands:

  nbdkit -fv --filter=cow memory size=512 --run 'qemu-img info $nbd'
  nbdkit -fv --filter=cache memory size=512 --run 'qemu-img info $nbd'

both fail with:

  nbdkit: memory[1]: error: realloc: Success

Initial git bisect pointed to commit 3166d2bcbfd2 (but I don't believe
that commit is the real cause, it merely exposes the bug).

The reason this happens is because the new behaviour after
commit 3166d2bcbfd2 is to round down the size of the underlying disk
to the cow/cache filter block size (4096 bytes).

=> The size of the disk is rounded down to 0.

=> The cow/cache filter requests a bitmap of size 0.

=> We call realloc (ptr, 0).

=> realloc returns NULL + errno == 0 in this case since it is not an
   error.

The current commit changes the realloc call so that it does not fail
in this case.  There are many other places in nbdkit where we call
realloc, and I did not vet any of them to see if similar bugs could be
present, but it is quite likely.

Note that the correct use of the cow/cache filter in this case is to
combine it with the truncate filter, eg:

  nbdkit -fv --filter=cow --filter=truncate memory size=512 round-up=4096
---
 common/bitmap/bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/bitmap/bitmap.c b/common/bitmap/bitmap.c
index caac916..6bf04dc 100644
--- a/common/bitmap/bitmap.c
+++ b/common/bitmap/bitmap.c
@@ -60,7 +60,7 @@ bitmap_resize (struct bitmap *bm, uint64_t new_size)
   new_bm_size = (size_t) new_bm_size_u64;
 
   new_bitmap = realloc (bm->bitmap, new_bm_size);
-  if (new_bitmap == NULL) {
+  if (new_bm_size && new_bitmap == NULL) {
     nbdkit_error ("realloc: %m");
     return -1;
   }
-- 
2.23.0

On 9/13/19 12:52 PM, Richard W.M. Jones wrote:
> The following commands:
> 
>   nbdkit -fv --filter=cow memory size=512 --run 'qemu-img info
$nbd'
>   nbdkit -fv --filter=cache memory size=512 --run 'qemu-img info
$nbd'
> 
> both fail with:
> 
>   nbdkit: memory[1]: error: realloc: Success
> 
> Initial git bisect pointed to commit 3166d2bcbfd2 (but I don't believe
> that commit is the real cause, it merely exposes the bug).
> 
> The reason this happens is because the new behaviour after
> commit 3166d2bcbfd2 is to round down the size of the underlying disk
> to the cow/cache filter block size (4096 bytes).
> 
> => The size of the disk is rounded down to 0.
> 
> => The cow/cache filter requests a bitmap of size 0.
> 
> => We call realloc (ptr, 0).
> 
> => realloc returns NULL + errno == 0 in this case since it is not an
>    error.
C11 says that whether realloc() returns NULL in this case is
unspecified; glibc happens to free the old pointer (if one was set) and
return NULL without setting errno (making it a successful alias for
free(ptr)), but other platforms treat it identically to their malloc(0)
(whether that mean returning a non-NULL pointer on BSD, or returning
NULL and setting errno because no 0-sized allocations are permitted - I
think HP-UX was such a platform).

In general, it's better to avoid 0-sized realloc in the first place
because of these differences in behavior, especially when ptr is
non-NULL (as you could end up leaking memory: glibc(ptr,0) frees ptr,
but a platform that forbids malloc(0) leaves ptr allocated).
> 
> The current commit changes the realloc call so that it does not fail
> in this case.  There are many other places in nbdkit where we call
> realloc, and I did not vet any of them to see if similar bugs could be
> present, but it is quite likely.
> 
> Note that the correct use of the cow/cache filter in this case is to
> combine it with the truncate filter, eg:
> 
>   nbdkit -fv --filter=cow --filter=truncate memory size=512 round-up=4096
> ---
>  common/bitmap/bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/bitmap/bitmap.c b/common/bitmap/bitmap.c
> index caac916..6bf04dc 100644
> --- a/common/bitmap/bitmap.c
> +++ b/common/bitmap/bitmap.c
> @@ -60,7 +60,7 @@ bitmap_resize (struct bitmap *bm, uint64_t new_size)
>    new_bm_size = (size_t) new_bm_size_u64;
>  
>    new_bitmap = realloc (bm->bitmap, new_bm_size);
> -  if (new_bitmap == NULL) {
> +  if (new_bm_size && new_bitmap == NULL) {
>      nbdkit_error ("realloc: %m");
>      return -1;
>    }
This avoids the failure, but is probably not as safe as avoiding the
0-length allocation in the first place.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org