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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190914/96800363/attachment.sig>