Libguestfs - Jan 2018 - Re: [PATCH nbdkit] filters: cache, cow: Handle bitmap overflow on 32 bit architectures.

When compiling on armv7 it becomes clear from the compiler warnings
that the current code is wrong.

The bitmap has to be allocated in virtual memory, so use size_t to
describe the length of the bitmap.  When changing the length of the
bitmap, compute the new size as an unsigned 64 bit int, and then check
whether or not it is too large to fit into size_t before casting it.
---
 filters/cache/cache.c | 13 ++++++++++---
 filters/cow/cow.c     | 13 ++++++++++---
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index 7410f0d..9473f2c 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -72,7 +72,7 @@ static int fd = -1;
 static uint8_t *bitmap;
 
 /* Size of the bitmap in bytes. */
-static uint64_t bm_size;
+static size_t bm_size;
 
 enum bm_entry {
   BLOCK_NOT_CACHED = 0,
@@ -167,7 +167,14 @@ blk_set_size (uint64_t new_size)
 {
   uint8_t *new_bm;
   const size_t old_bm_size = bm_size;
-  size_t new_bm_size = DIV_ROUND_UP (new_size, BLKSIZE*8/2);
+  uint64_t new_bm_size_u64 = DIV_ROUND_UP (new_size, BLKSIZE*8/2);
+  size_t new_bm_size;
+
+  if (new_bm_size_u64 > SIZE_MAX) {
+    nbdkit_error ("bitmap too large for this architecture");
+    return -1;
+  }
+  new_bm_size = (size_t) new_bm_size_u64;
 
   new_bm = realloc (bitmap, new_bm_size);
   if (new_bm == NULL) {
@@ -179,7 +186,7 @@ blk_set_size (uint64_t new_size)
   if (old_bm_size < new_bm_size)
     memset (&bitmap[old_bm_size], 0, new_bm_size-old_bm_size);
 
-  nbdkit_debug ("cache: bitmap resized to %" PRIu64 "
bytes", new_bm_size);
+  nbdkit_debug ("cache: bitmap resized to %zu bytes", new_bm_size);
 
   if (ftruncate (fd, new_size) == -1) {
     nbdkit_error ("ftruncate: %m");
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 2b023af..5c2fcd0 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -97,7 +97,7 @@ static int fd = -1;
 static uint8_t *bitmap;
 
 /* Size of the bitmap in bytes. */
-static uint64_t bm_size;
+static size_t bm_size;
 
 static void
 cow_load (void)
@@ -153,7 +153,14 @@ blk_set_size (uint64_t new_size)
 {
   uint8_t *new_bm;
   const size_t old_bm_size = bm_size;
-  size_t new_bm_size = DIV_ROUND_UP (new_size, BLKSIZE*8);
+  uint64_t new_bm_size_u64 = DIV_ROUND_UP (new_size, BLKSIZE*8);
+  size_t new_bm_size;
+
+  if (new_bm_size_u64 > SIZE_MAX) {
+    nbdkit_error ("bitmap too large for this architecture");
+    return -1;
+  }
+  new_bm_size = (size_t) new_bm_size_u64;
 
   new_bm = realloc (bitmap, new_bm_size);
   if (new_bm == NULL) {
@@ -165,7 +172,7 @@ blk_set_size (uint64_t new_size)
   if (old_bm_size < new_bm_size)
     memset (&bitmap[old_bm_size], 0, new_bm_size-old_bm_size);
 
-  nbdkit_debug ("cow: bitmap resized to %" PRIu64 " bytes",
new_bm_size);
+  nbdkit_debug ("cow: bitmap resized to %zu bytes", new_bm_size);
 
   if (ftruncate (fd, new_size) == -1) {
     nbdkit_error ("ftruncate: %m");
-- 
2.13.2

On 01/26/2018 07:17 AM, Richard W.M. Jones wrote:
> When compiling on armv7 it becomes clear from the compiler warnings
> that the current code is wrong.
> 
> The bitmap has to be allocated in virtual memory, so use size_t to
> describe the length of the bitmap.  When changing the length of the
> bitmap, compute the new size as an unsigned 64 bit int, and then check
> whether or not it is too large to fit into size_t before casting it.
Indeed, size_t is a sane limit (the only way to get a larger map would
be using an mmap'd file on disk, so that we can use 64-bit off_t instead
of 32-bit size_t).  We could also play games with having the granularity
get larger (doubling the size each bit represents lets you cache a
larger disk image).  But erroring out for now, and leaving such
enhancements for a future contributor, is fine by me.

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

On 01/26/2018 07:53 AM, Eric Blake wrote:
> On 01/26/2018 07:17 AM, Richard W.M. Jones wrote:
>> When compiling on armv7 it becomes clear from the compiler warnings
>> that the current code is wrong.
>>
>> The bitmap has to be allocated in virtual memory, so use size_t to
>> describe the length of the bitmap.  When changing the length of the
>> bitmap, compute the new size as an unsigned 64 bit int, and then check
>> whether or not it is too large to fit into size_t before casting it.
> 
> Indeed, size_t is a sane limit (the only way to get a larger map would
> be using an mmap'd file on disk, so that we can use 64-bit off_t
instead
> of 32-bit size_t).
and using a second layer of caching to swap in windows of that file as
needed, as the entire file won't fit in memory at once.  That's a lot of
bookkeeping overhead, for something that is less and less useful as more
people use 64-bit machines.

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