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

v1 was here:
https://www.redhat.com/archives/libguestfs/2019-September/msg00100.html

In v2 I've changed the patch so it avoids calling realloc at all in
this case.

The patch is a bit longer this way.  But I don't see any other
alternative if we are to avoid having a "realloc wrapper" of some kind
that we use everywhere, which I guess we should avoid because it makes
plugins just that bit harder to write and we want them to be as easy
to write and accessible as possible.

Also:

 - Clean up the commit message.

 - Add a simple regression test.

Rich.

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'
  nbdkit -fv --filter=cow null --run 'qemu-img info $nbd'

all fail with:

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

Initial git bisect pointed to commit 3166d2bcbfd2 (this is not 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.

=> Size of the underlying disk is 512 or 0 bytes,
   block size is 4096 bytes.

=> Size of the disk is rounded down to 0.

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

=> bitmap_resize calls realloc (ptr, 0).

=> The glibc implementation of realloc returns NULL + errno == 0.
   (Other realloc implementations can behave differently.)

=> This is not an error, but the existing code thinks it is because of
   the NULL return.

This commit changes the code so it doesn't bother to call realloc if
the new bitmap size would be 0.

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 in passing that the correct way to use the cow/cache filter with
a disk which isn't a multiple of the block size is to combine it with
the truncate filter, eg:

  nbdkit -fv --filter=cow --filter=truncate memory size=512 round-up=4096

Thanks: Eric Blake
---
 common/bitmap/bitmap.c | 14 ++++++++++----
 tests/Makefile.am      |  2 ++
 tests/test-cow-null.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/common/bitmap/bitmap.c b/common/bitmap/bitmap.c
index caac916..2cc625c 100644
--- a/common/bitmap/bitmap.c
+++ b/common/bitmap/bitmap.c
@@ -59,10 +59,16 @@ 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) {
-    nbdkit_error ("realloc: %m");
-    return -1;
+  if (new_bm_size > 0) {
+    new_bitmap = realloc (bm->bitmap, new_bm_size);
+    if (new_bitmap == NULL) {
+      nbdkit_error ("realloc: %m");
+      return -1;
+    }
+  }
+  else {
+    free (bm->bitmap);
+    new_bitmap = NULL;
   }
   bm->bitmap = new_bitmap;
   bm->size = new_bm_size;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f54597b..9eec75e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -50,6 +50,7 @@ EXTRA_DIST = \
 	test-cacheextents.sh \
 	test-captive.sh \
 	test-cow.sh \
+	test-cow-null.sh \
 	test-cxx.sh \
 	test-data-7E.sh \
 	test-data-base64.sh \
@@ -960,6 +961,7 @@ TESTS += test-cacheextents.sh
 if HAVE_GUESTFISH
 TESTS += test-cow.sh
 endif HAVE_GUESTFISH
+TESTS += test-cow-null.sh
 
 # delay filter tests.
 TESTS += test-shutdown.sh
diff --git a/tests/test-cow-null.sh b/tests/test-cow-null.sh
new file mode 100755
index 0000000..fd6c04f
--- /dev/null
+++ b/tests/test-cow-null.sh
@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019 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.
+
+# Regression test for placing a cow filter over a < 4K sized device,
+# which was broken in nbdkit 1.14.0.
+
+source ./functions.sh
+set -e
+set -x
+
+requires qemu-img --version
+
+nbdkit -fv --filter=cow null --run 'qemu-img info $nbd'
-- 
2.23.0

On 9/15/19 6:55 AM, 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'
>   nbdkit -fv --filter=cow null --run 'qemu-img info $nbd'
> 
> all fail with:
> 
>   nbdkit: memory[1]: error: realloc: Success
> This commit changes the code so it doesn't bother to call realloc if
> the new bitmap size would be 0.
ACK
> 
> 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.
I did an audit of them; you caught the only culprit:

common/include/get-current-dir-name.h - immune (used +1)
common/regions/regions.c - immune (used +1)
common/sparse/sparse.c - immune (used +1)
filters/readahead/readahead.c - immune (called with non-zero count)
plugins/floppy/directory-lfn.c - immune (used +1)
plugins/floppy/virtual-floppy.c - immune (used +1)
plugins/iso/iso.c - immune (used +1)
plugins/partitioning/partitioning.c - immune (used +1)
plugins/sh/call.c - immune (called with non-zero bufalloc)
plugins/split/split.c - immune (used +1)
plugins/ssh/ssh.c - immune (used +1)
server/extents.c - immune (called with non-zero new_extents)
server/sockets.c - immune (called with non-zero nr_socks)
server/threadlocal.c - immune (called with non-zero count)
tests/test-layers.c - immune (called with non-zero allocated)
wrapper.c - immune (used +1)

> 
> Note in passing that the correct way to use the cow/cache filter with
> a disk which isn't a multiple of the block size is to combine it with
> the truncate filter, eg:
> 
>   nbdkit -fv --filter=cow --filter=truncate memory size=512 round-up=4096
> 
> Thanks: Eric Blake
> ---

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