[libvirt PATCH 0/3] Random fixes
I found that I forgot to send the bhyve patch with the rest of the cppcheck patches. And I also found some other patches. Ján Tomko (3): bhyve: fix NULL pointer check position virbitmap: remove redundant mem_alloc util: bitmap: use g_new0/g_free src/bhyve/bhyve_parse_command.c | 14 +++++++------- src/util/virbitmap.c | 32 +++++++++----------------------- 2 files changed, 16 insertions(+), 30 deletions(-) -- 2.26.2
src/bhyve/bhyve_parse_command.c:437:9: warning: Either the condition '!config' is redundant or there is possible null pointer dereference: config. [nullPointerRedundantCheck] src/bhyve/bhyve_parse_command.c:280:23: warning: Either the condition '!separator' is redundant or there is pointer arithmetic with NULL pointer. [nullPointerArithmeticRedundantCheck] Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/bhyve/bhyve_parse_command.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index b2d2280872..5e9cf7ba13 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -277,11 +277,11 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def, char *type = NULL; separator = strchr(arg, ','); + + if (!separator) + goto error; + param = separator + 1; - - if (!separator) - goto error; - type = g_strndup(arg, separator - arg); /* Only support com%d */ @@ -434,14 +434,14 @@ bhyveParsePCIDisk(virDomainDefPtr def, disk->info.addr.pci.slot = pcislot; disk->info.addr.pci.function = function; + if (!config) + goto error; + if (STRPREFIX(config, "/dev/")) disk->src->type = VIR_STORAGE_TYPE_BLOCK; else disk->src->type = VIR_STORAGE_TYPE_FILE; - if (!config) - goto error; - separator = strchr(config, ','); if (separator) disk->src->path = g_strndup(config, separator - config); -- 2.26.2
Ján Tomko wrote:
src/bhyve/bhyve_parse_command.c:437:9: warning: Either the condition '!config' is redundant or there is possible null pointer dereference: config. [nullPointerRedundantCheck]
src/bhyve/bhyve_parse_command.c:280:23: warning: Either the condition '!separator' is redundant or there is pointer arithmetic with NULL pointer. [nullPointerArithmeticRedundantCheck]
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/bhyve/bhyve_parse_command.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index b2d2280872..5e9cf7ba13 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -277,11 +277,11 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def, char *type = NULL;
separator = strchr(arg, ','); + + if (!separator) + goto error; + param = separator + 1; - - if (!separator) - goto error; - type = g_strndup(arg, separator - arg);
/* Only support com%d */ @@ -434,14 +434,14 @@ bhyveParsePCIDisk(virDomainDefPtr def, disk->info.addr.pci.slot = pcislot; disk->info.addr.pci.function = function;
+ if (!config) + goto error; + if (STRPREFIX(config, "/dev/")) disk->src->type = VIR_STORAGE_TYPE_BLOCK; else disk->src->type = VIR_STORAGE_TYPE_FILE;
- if (!config) - goto error; - separator = strchr(config, ','); if (separator) disk->src->path = g_strndup(config, separator - config);
Reviewed-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
-- 2.26.2
Roman Bogorodskiy
We have two variables tracking the size of the map: map_len and mem_alloc. Remove mem_alloc as well as code keeping them in sync. Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 917426c8d7dd26f13142fc4c5c1a8a19137ac647 --- src/util/virbitmap.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 60fd8491dd..4c205016ff 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -34,7 +34,6 @@ struct _virBitmap { size_t nbits; size_t map_len; - size_t map_alloc; /* Note that code below depends on the fact that unused bits of the bitmap * are not set. Any function decreasing the size of the map needs clear @@ -79,7 +78,6 @@ virBitmapNewQuiet(size_t size) bitmap->nbits = size; bitmap->map_len = sz; - bitmap->map_alloc = sz; return bitmap; } @@ -197,13 +195,12 @@ virBitmapExpand(virBitmapPtr map, /* resize the memory if necessary */ if (map->map_len < new_len) { - if (VIR_RESIZE_N(map->map, map->map_alloc, map->map_len, + if (VIR_RESIZE_N(map->map, map->map_len, map->map_len, new_len - map->map_len) < 0) return -1; } map->nbits = b + 1; - map->map_len = new_len; return 0; } @@ -1317,13 +1314,10 @@ virBitmapShrink(virBitmapPtr map, nb = map->nbits % VIR_BITMAP_BITS_PER_UNIT; map->map[nl] &= ((1UL << nb) - 1); - toremove = map->map_alloc - (nl + 1); + toremove = map->map_len - (nl + 1); if (toremove == 0) return; - VIR_SHRINK_N(map->map, map->map_alloc, toremove); - - /* length needs to be fixed as well */ - map->map_len = map->map_alloc; + VIR_SHRINK_N(map->map, map->map_len, toremove); } -- 2.26.2
On Mon, Aug 03, 2020 at 19:18:52 +0200, Ján Tomko wrote:
We have two variables tracking the size of the map: map_len and mem_alloc.
Remove mem_alloc as well as code keeping them in sync.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 917426c8d7dd26f13142fc4c5c1a8a19137ac647 --- src/util/virbitmap.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 60fd8491dd..4c205016ff 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c
[...]
@@ -197,13 +195,12 @@ virBitmapExpand(virBitmapPtr map,
/* resize the memory if necessary */ if (map->map_len < new_len) { - if (VIR_RESIZE_N(map->map, map->map_alloc, map->map_len, + if (VIR_RESIZE_N(map->map, map->map_len, map->map_len, new_len - map->map_len) < 0) return -1;
* VIR_RESIZE_N: * @ptr: pointer to hold address of allocated memory * @alloc: variable tracking number of elements currently allocated * @count: number of elements currently in use * @add: minimum number of elements to additionally support * * Blindly using VIR_EXPAND_N(array, alloc, 1) in a loop scales * quadratically, because every iteration must copy contents from * all prior iterations. But amortized linear scaling can be achieved * by tracking allocation size separately from the number of used * elements, and growing geometrically only as needed. * * If 'count' + 'add' is larger than 'alloc', then geometrically reallocate * the array of 'alloc' elements, each sizeof(*ptr) bytes long, and store * the address of allocated memory in 'ptr' and the new size in 'alloc'. * The new elements are filled with zero.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -68,13 +68,8 @@ virBitmapNewQuiet(size_t size) sz = VIR_DIV_UP(size, VIR_BITMAP_BITS_PER_UNIT); - if (VIR_ALLOC_QUIET(bitmap) < 0) - return NULL; - - if (VIR_ALLOC_N_QUIET(bitmap->map, sz) < 0) { - VIR_FREE(bitmap); - return NULL; - } + bitmap = g_new0(virBitmap, 1); + bitmap->map = g_new0(unsigned long, sz); bitmap->nbits = size; bitmap->map_len = sz; @@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map); + g_free(bitmap); } @@ -779,9 +773,7 @@ virBitmapToData(virBitmapPtr bitmap, else len = (len + CHAR_BIT) / CHAR_BIT; - if (VIR_ALLOC_N(*data, len) < 0) - return -1; - + *data = g_new0(unsigned char, len); *dataLen = len; virBitmapToDataBuf(bitmap, *data, *dataLen); -- 2.26.2
On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c
[...]
@@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map);
Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If a caller uses a stale pointer, it will crash on dereferencing this part.
On 8/3/20 1:29 PM, Peter Krempa wrote:
On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c [...]
@@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map); Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If a caller uses a stale pointer, it will crash on dereferencing this part.
This is an argument for NULLifying *all* pointers when they are freed (yes, even things on the stack).
On Mon, Aug 03, 2020 at 07:29:19PM +0200, Peter Krempa wrote:
On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c
[...]
@@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map);
Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If a caller uses a stale pointer, it will crash on dereferencing this part.
There's no strong reason to do that in a vir*Free() function, since 'bitmap' itself is being freed. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 8/4/20 5:09 AM, Daniel P. Berrangé wrote:
On Mon, Aug 03, 2020 at 07:29:19PM +0200, Peter Krempa wrote:
On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c [...]
@@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map); Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If a caller uses a stale pointer, it will crash on dereferencing this part. There's no strong reason to do that in a vir*Free() function, since 'bitmap' itself is being freed.
I think he was pointing out that if you zero out the contents of the virBitmap object before you free it, then a caller who mistakenly keeps around a pointer to "bitmap" after calling virBitmapFree(bitmap), and then attempts to use it, thus causing a dereference of bitmap->map, will get an immediate segfault rather than using some chunk of memory that may have already been allocated to something else. This is a useful concept, but unless we do it *everywhere*, making a special case here and there isn't going to have much effect (that was the implication of my original response) - it's kind of emptying the ocean with a tea spoon. (and also it makes the code uglier and more confusing). Now if we could efficiently zero out all blocks of memory as they were freed, then maybe there would be some real bug catching value.
On Tue, Aug 04, 2020 at 23:15:26 -0400, Laine Stump wrote:
On 8/4/20 5:09 AM, Daniel P. Berrangé wrote:
On Mon, Aug 03, 2020 at 07:29:19PM +0200, Peter Krempa wrote:
On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virbitmap.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 4c205016ff..f7dd5d05ad 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c [...]
@@ -126,10 +121,9 @@ virBitmapNewEmpty(void) void virBitmapFree(virBitmapPtr bitmap) { - if (bitmap) { - VIR_FREE(bitmap->map); - VIR_FREE(bitmap); - } + if (bitmap) + g_free(bitmap->map); Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If a caller uses a stale pointer, it will crash on dereferencing this part. There's no strong reason to do that in a vir*Free() function, since 'bitmap' itself is being freed.
I think he was pointing out that if you zero out the contents of the virBitmap object before you free it, then a caller who mistakenly keeps around a pointer to "bitmap" after calling virBitmapFree(bitmap), and then attempts to use it, thus causing a dereference of bitmap->map, will get an immediate segfault rather than using some chunk of memory that may have already been allocated to something else.
Yes, exactly. On the other hand I see that it's mostly pointless. But please remember that we also cleared the pointer in our own implementation of VIR_AUTOFREE, which was even more pointless. Thankfully glib doesn't do such silly things.
This is a useful concept, but unless we do it *everywhere*, making a special case here and there isn't going to have much effect (that was the implication of my original response) - it's kind of emptying the ocean with a tea spoon. (and also it makes the code uglier and more confusing). Now if we could efficiently zero out all blocks of memory as they were freed, then maybe there would be some real bug catching value.
I agree. Unfortunately this just clears out pointers, but any other variables stay the same so other things may broke terribly. Since the memory can be overwritten right away after being freed any NULing of memory is not a 100% fix. I still think that it's worth though in many cases.
participants (5)
-
Daniel P. Berrangé -
Ján Tomko -
Laine Stump -
Peter Krempa -
Roman Bogorodskiy