From a3c2f12830683e285e1ef32d459717dcdf9b70c6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 29 Jul 2021 15:53:27 +0200 Subject: [PATCH 1/8] vl: introduce machine_merge_property It will be used to parse smp-opts config groups from configuration files. The point to note is that it does not steal a reference from the caller. This is better because this function will be called from qemu_config_foreach's callback; qemu_config_foreach does not cede its reference to the qdict to the callback, and wants to free it. To balance that extra reference, machine_parse_property_opt now needs a qobject_unref. Signed-off-by: Paolo Bonzini --- softmmu/vl.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 4dee472c79..93aef8e747 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1534,23 +1534,36 @@ static void machine_help_func(const QDict *qdict) } } +static void +machine_merge_property(const char *propname, QDict *prop, Error **errp) +{ + QDict *opts; + + opts = qdict_new(); + /* Preserve the caller's reference to prop. */ + qobject_ref(prop); + qdict_put(opts, propname, prop); + keyval_merge(machine_opts_dict, opts, errp); + qobject_unref(opts); +} + static void machine_parse_property_opt(QemuOptsList *opts_list, const char *propname, const char *arg, Error **errp) { - QDict *opts, *prop; + QDict *prop = NULL; bool help = false; - ERRP_GUARD(); prop = keyval_parse(arg, opts_list->implied_opt_name, &help, errp); if (help) { qemu_opts_print_help(opts_list, true); exit(0); } - opts = qdict_new(); - qdict_put(opts, propname, prop); - keyval_merge(machine_opts_dict, opts, errp); - qobject_unref(opts); + if (!prop) { + return; + } + machine_merge_property(propname, prop, errp); + qobject_unref(prop); } static const char *pid_file; From e4383ca240d804bf1c472ed004d6c7b8a505fc63 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 29 Jul 2021 16:03:43 +0200 Subject: [PATCH 2/8] vl: stop recording -smp in QemuOpts -readconfig is still recording SMP options in QemuOpts instead of using machine_opts_dict. This means that SMP options from -readconfig are ignored. Just stop using QemuOpts for -smp, making it return false for is_qemuopts_group. Configuration files will merge the values in machine_opts_dict using the new function machine_merge_property. At the same time, fix -mem-prealloc which looked at QemuOpts to find the number of guest CPUs, which it used as the default number of preallocation threads. Signed-off-by: Paolo Bonzini --- softmmu/vl.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 93aef8e747..5ca11e7469 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -31,6 +31,7 @@ #include "qapi/compat-policy.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" +#include "qapi/qmp/qstring.h" #include "qapi/qmp/qjson.h" #include "qemu-version.h" #include "qemu/cutils.h" @@ -2166,7 +2167,8 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp) static bool is_qemuopts_group(const char *group) { if (g_str_equal(group, "object") || - g_str_equal(group, "machine")) { + g_str_equal(group, "machine") || + g_str_equal(group, "smp-opts")) { return false; } return true; @@ -2186,6 +2188,8 @@ static void qemu_record_config_group(const char *group, QDict *dict, */ assert(!from_json); keyval_merge(machine_opts_dict, dict, errp); + } else if (g_str_equal(group, "smp-opts")) { + machine_merge_property("smp", dict, &error_fatal); } else { abort(); } @@ -2452,13 +2456,15 @@ static void qemu_validate_options(const QDict *machine_opts) static void qemu_process_sugar_options(void) { if (mem_prealloc) { - char *val; - - val = g_strdup_printf("%d", - (uint32_t) qemu_opt_get_number(qemu_find_opts_singleton("smp-opts"), "cpus", 1)); - object_register_sugar_prop("memory-backend", "prealloc-threads", val, - false); - g_free(val); + QObject *smp = qdict_get(machine_opts_dict, "smp"); + if (smp && qobject_type(smp) == QTYPE_QDICT) { + QObject *cpus = qdict_get(qobject_to(QDict, smp), "cpus"); + if (cpus && qobject_type(cpus) == QTYPE_QSTRING) { + const char *val = qstring_get_str(qobject_to(QString, cpus)); + object_register_sugar_prop("memory-backend", "prealloc-threads", + val, false); + } + } object_register_sugar_prop("memory-backend", "prealloc", "on", false); } From d4b3d152ee005825520dc171e1e650174ae5ebe6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 27 Jul 2021 17:50:09 +0200 Subject: [PATCH 3/8] coverity-model: update address_space_read/write models Use void * for consistency with the actual function; provide a model for MemoryRegionCache functions and for address_space_rw. These let Coverity understand the bounds of the data that various functions read and write even at very high levels of inlining (e.g. pci_dma_read). Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 48 ++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 2c0346ff25..e1bdf0ad84 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -45,9 +45,10 @@ typedef struct va_list_str *va_list; /* exec.c */ typedef struct AddressSpace AddressSpace; +typedef struct MemoryRegionCache MemoryRegionCache; typedef uint64_t hwaddr; typedef uint32_t MemTxResult; -typedef uint64_t MemTxAttrs; +typedef struct MemTxAttrs {} MemTxAttrs; static void __bufwrite(uint8_t *buf, ssize_t len) { @@ -67,9 +68,40 @@ static void __bufread(uint8_t *buf, ssize_t len) int last = buf[len-1]; } +MemTxResult address_space_read_cached(MemoryRegionCache *cache, hwaddr addr, + MemTxAttrs attrs, + void *buf, int len) +{ + MemTxResult result; + // TODO: investigate impact of treating reads as producing + // tainted data, with __coverity_tainted_data_argument__(buf). + __bufwrite(buf, len); + return result; +} + +MemTxResult address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, + MemTxAttrs attrs, + const void *buf, int len) +{ + MemTxResult result; + __bufread(buf, len); + return result; +} + +MemTxResult address_space_rw_cached(MemoryRegionCache *cache, hwaddr addr, + MemTxAttrs attrs, + void *buf, int len, bool is_write) +{ + if (is_write) { + return address_space_write_cached(cache, addr, attrs, buf, len); + } else { + return address_space_read_cached(cache, addr, attrs, buf, len); + } +} + MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, - uint8_t *buf, int len) + void *buf, int len) { MemTxResult result; // TODO: investigate impact of treating reads as producing @@ -80,13 +112,23 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, - const uint8_t *buf, int len) + const void *buf, int len) { MemTxResult result; __bufread(buf, len); return result; } +MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, + MemTxAttrs attrs, + void *buf, int len, bool is_write) +{ + if (is_write) { + return address_space_write(as, addr, attrs, buf, len); + } else { + return address_space_read(as, addr, attrs, buf, len); + } +} /* Tainting */ From 243a545bffc7e86c0f5ae97c0f7d32c079ab78a3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 27 Jul 2021 17:54:15 +0200 Subject: [PATCH 4/8] coverity-model: make g_free a synonym of free Recently, Coverity has started complaining about using g_free() to free memory areas allocated by GLib functions not included in model.c, such as g_strfreev. This unfortunately goes against the GLib documentation, which suggests that g_malloc() should be matched with g_free() and plain malloc() with free(); since GLib 2.46 however g_malloc() is hardcoded to always use the system malloc implementation, and g_free is just "free" plus a tracepoint. Therefore, this should not cause any problem in practice. Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index e1bdf0ad84..8e64a84c5a 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -186,7 +186,7 @@ void *g_malloc_n(size_t nmemb, size_t size) sz = nmemb * size; ptr = __coverity_alloc__(sz); __coverity_mark_as_uninitialized_buffer__(ptr); - __coverity_mark_as_afm_allocated__(ptr, "g_free"); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; } @@ -200,7 +200,7 @@ void *g_malloc0_n(size_t nmemb, size_t size) sz = nmemb * size; ptr = __coverity_alloc__(sz); __coverity_writeall0__(ptr); - __coverity_mark_as_afm_allocated__(ptr, "g_free"); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; } @@ -218,14 +218,14 @@ void *g_realloc_n(void *ptr, size_t nmemb, size_t size) * model that. See Coverity's realloc() model */ __coverity_writeall__(ptr); - __coverity_mark_as_afm_allocated__(ptr, "g_free"); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; } void g_free(void *ptr) { __coverity_free__(ptr); - __coverity_mark_as_afm_freed__(ptr, "g_free"); + __coverity_mark_as_afm_freed__(ptr, AFM_free); } /* @@ -328,7 +328,7 @@ char *g_strdup(const char *s) __coverity_string_null_sink__(s); __coverity_string_size_sink__(s); dup = __coverity_alloc_nosize__(); - __coverity_mark_as_afm_allocated__(dup, "g_free"); + __coverity_mark_as_afm_allocated__(dup, AFM_free); for (i = 0; (dup[i] = s[i]); i++) ; return dup; } @@ -362,7 +362,7 @@ char *g_strdup_printf(const char *format, ...) s = __coverity_alloc_nosize__(); __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, "g_free"); + __coverity_mark_as_afm_allocated__(s, AFM_free); return s; } @@ -375,11 +375,10 @@ char *g_strdup_vprintf(const char *format, va_list ap) __coverity_string_size_sink__(format); ch = *format; - ch = *(char *)ap; s = __coverity_alloc_nosize__(); __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, "g_free"); + __coverity_mark_as_afm_allocated__(s, AFM_free); return len; } @@ -395,7 +394,7 @@ char *g_strconcat(const char *s, ...) s = __coverity_alloc_nosize__(); __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, "g_free"); + __coverity_mark_as_afm_allocated__(s, AFM_free); return s; } From 96915d638cb83aa139e39096815b8dd9832f264b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 27 Jul 2021 17:56:04 +0200 Subject: [PATCH 5/8] coverity-model: remove model for more allocation functions These models are not needed anymore now that Coverity does not check anymore that the result is used with "g_free". Coverity understands GCC attributes and uses them to detect leaks. Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 105 +--------------------------------- 1 file changed, 1 insertion(+), 104 deletions(-) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 8e64a84c5a..1a5f39d2ae 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -263,7 +263,7 @@ void *g_try_realloc_n(void *ptr, size_t nmemb, size_t size) return g_realloc_n(ptr, nmemb, size); } -/* Trivially derive the g_FOO() from the g_FOO_n() */ +/* Derive the g_FOO() from the g_FOO_n() */ void *g_malloc(size_t size) { @@ -295,109 +295,6 @@ void *g_try_realloc(void *ptr, size_t size) return g_try_realloc_n(ptr, 1, size); } -/* Other memory allocation functions */ - -void *g_memdup(const void *ptr, unsigned size) -{ - unsigned char *dup; - unsigned i; - - if (!ptr) { - return NULL; - } - - dup = g_malloc(size); - for (i = 0; i < size; i++) - dup[i] = ((unsigned char *)ptr)[i]; - return dup; -} - -/* - * GLib string allocation functions - */ - -char *g_strdup(const char *s) -{ - char *dup; - size_t i; - - if (!s) { - return NULL; - } - - __coverity_string_null_sink__(s); - __coverity_string_size_sink__(s); - dup = __coverity_alloc_nosize__(); - __coverity_mark_as_afm_allocated__(dup, AFM_free); - for (i = 0; (dup[i] = s[i]); i++) ; - return dup; -} - -char *g_strndup(const char *s, size_t n) -{ - char *dup; - size_t i; - - __coverity_negative_sink__(n); - - if (!s) { - return NULL; - } - - dup = g_malloc(n + 1); - for (i = 0; i < n && (dup[i] = s[i]); i++) ; - dup[i] = 0; - return dup; -} - -char *g_strdup_printf(const char *format, ...) -{ - char ch, *s; - size_t len; - - __coverity_string_null_sink__(format); - __coverity_string_size_sink__(format); - - ch = *format; - - s = __coverity_alloc_nosize__(); - __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, AFM_free); - return s; -} - -char *g_strdup_vprintf(const char *format, va_list ap) -{ - char ch, *s; - size_t len; - - __coverity_string_null_sink__(format); - __coverity_string_size_sink__(format); - - ch = *format; - - s = __coverity_alloc_nosize__(); - __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, AFM_free); - - return len; -} - -char *g_strconcat(const char *s, ...) -{ - char *s; - - /* - * Can't model: last argument must be null, the others - * null-terminated strings - */ - - s = __coverity_alloc_nosize__(); - __coverity_writeall__(s); - __coverity_mark_as_afm_allocated__(s, AFM_free); - return s; -} - /* Other glib functions */ typedef struct pollfd GPollFD; From 05ad6857a57238c27df84f6c0c1943dd162a82ad Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 27 Jul 2021 17:55:41 +0200 Subject: [PATCH 6/8] coverity-model: clean up the models for array allocation functions sz is only used in one place, so replace it with nmemb * size in that one place. Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 1a5f39d2ae..2d384bdd79 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -178,13 +178,11 @@ uint8_t replay_get_byte(void) void *g_malloc_n(size_t nmemb, size_t size) { - size_t sz; void *ptr; __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); __coverity_mark_as_uninitialized_buffer__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -192,13 +190,11 @@ void *g_malloc_n(size_t nmemb, size_t size) void *g_malloc0_n(size_t nmemb, size_t size) { - size_t sz; void *ptr; __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); __coverity_writeall0__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -206,13 +202,10 @@ void *g_malloc0_n(size_t nmemb, size_t size) void *g_realloc_n(void *ptr, size_t nmemb, size_t size) { - size_t sz; - __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; __coverity_escape__(ptr); - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); /* * Memory beyond the old size isn't actually initialized. Can't * model that. See Coverity's realloc() model From 0da41187dfda6abecbcbc237471254ab614e063d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 28 Jul 2021 19:12:22 +0200 Subject: [PATCH 7/8] coverity-model: constrain g_malloc/g_malloc0/g_realloc as never returning NULL g_malloc/g_malloc0/g_realloc only return NULL if the size is 0; we do not need to cover that in the model, and so far have expected __coverity_alloc__ to model a non-NULL return value. But that apparently does not work anymore, so add some extra conditionals that invoke __coverity_panic__ for NULL pointers. Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 2d384bdd79..028f13e9e3 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -183,6 +183,9 @@ void *g_malloc_n(size_t nmemb, size_t size) __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); ptr = __coverity_alloc__(nmemb * size); + if (!ptr) { + __coverity_panic__(); + } __coverity_mark_as_uninitialized_buffer__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -195,6 +198,9 @@ void *g_malloc0_n(size_t nmemb, size_t size) __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); ptr = __coverity_alloc__(nmemb * size); + if (!ptr) { + __coverity_panic__(); + } __coverity_writeall0__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -206,6 +212,9 @@ void *g_realloc_n(void *ptr, size_t nmemb, size_t size) __coverity_negative_sink__(size); __coverity_escape__(ptr); ptr = __coverity_alloc__(nmemb * size); + if (!ptr) { + __coverity_panic__(); + } /* * Memory beyond the old size isn't actually initialized. Can't * model that. See Coverity's realloc() model From e17bdaab2b36db54f0214a14f394fa773cee58df Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 27 Jul 2021 18:03:16 +0200 Subject: [PATCH 8/8] coverity-model: write models fully for non-array allocation functions Coverity seems to have issues figuring out the properties of g_malloc0 and other non *_n functions. While this was "fixed" by removing the custom second argument to __coverity_mark_as_afm_allocated__, inline the code from the array-based allocation functions to avoid future issues. Signed-off-by: Paolo Bonzini --- scripts/coverity-scan/model.c | 57 +++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 028f13e9e3..9d4fba53d9 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -269,32 +269,77 @@ void *g_try_realloc_n(void *ptr, size_t nmemb, size_t size) void *g_malloc(size_t size) { - return g_malloc_n(1, size); + void *ptr; + + __coverity_negative_sink__(size); + ptr = __coverity_alloc__(size); + if (!ptr) { + __coverity_panic__(); + } + __coverity_mark_as_uninitialized_buffer__(ptr); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); + return ptr; } void *g_malloc0(size_t size) { - return g_malloc0_n(1, size); + void *ptr; + + __coverity_negative_sink__(size); + ptr = __coverity_alloc__(size); + if (!ptr) { + __coverity_panic__(); + } + __coverity_writeall0__(ptr); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); + return ptr; } void *g_realloc(void *ptr, size_t size) { - return g_realloc_n(ptr, 1, size); + __coverity_negative_sink__(size); + __coverity_escape__(ptr); + ptr = __coverity_alloc__(size); + if (!ptr) { + __coverity_panic__(); + } + /* + * Memory beyond the old size isn't actually initialized. Can't + * model that. See Coverity's realloc() model + */ + __coverity_writeall__(ptr); + __coverity_mark_as_afm_allocated__(ptr, AFM_free); + return ptr; } void *g_try_malloc(size_t size) { - return g_try_malloc_n(1, size); + int nomem; + + if (nomem) { + return NULL; + } + return g_malloc(size); } void *g_try_malloc0(size_t size) { - return g_try_malloc0_n(1, size); + int nomem; + + if (nomem) { + return NULL; + } + return g_malloc0(size); } void *g_try_realloc(void *ptr, size_t size) { - return g_try_realloc_n(ptr, 1, size); + int nomem; + + if (nomem) { + return NULL; + } + return g_realloc(ptr, size); } /* Other glib functions */