Re: [PATCH v2 08/20] mcd: Implement server connection API

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Apr 30, 2025 at 07:27:29AM +0200, Mario Fleischmann wrote:
> This commit implements the necessary operations required to establish
> a connection with the MCD server:
> 
> * query information about the server
> * connect to "
> * disconnect from "
> 
> Signed-off-by: Mario Fleischmann <mario.fleischmann@lauterbach.com>
> ---
>  mcd/mcd_qapi.c         |  13 +++
>  mcd/mcd_qapi.h         |   2 +
>  mcd/mcd_server.c       | 110 +++++++++++++++++++++-
>  mcd/mcd_stub.c         |  98 ++++++++++++++++++++
>  qapi/mcd.json          | 205 +++++++++++++++++++++++++++++++++++++++++
>  tests/qtest/mcd-test.c |  96 +++++++++++++++++++
>  tests/qtest/mcd-util.c |  60 ++++++++++++
>  tests/qtest/mcd-util.h |   9 ++
>  8 files changed, 588 insertions(+), 5 deletions(-)
> 
> diff --git a/mcd/mcd_qapi.c b/mcd/mcd_qapi.c
> index 9a99866..d2a2926 100644
> --- a/mcd/mcd_qapi.c
> +++ b/mcd/mcd_qapi.c


> +MCDQryServersResult *qmp_mcd_qry_servers(const char *host, bool running,
> +                                         uint32_t start_index,
> +                                         uint32_t num_servers, Error **errp)
> +{
> +    MCDServerInfoList **tailp;
> +    MCDServerInfo *info;
> +    mcd_server_info_st *server_info = NULL;
> +    bool query_num_only = num_servers == 0;
> +    MCDQryServersResult *result = g_malloc0(sizeof(*result));
> +
> +    if (!query_num_only) {
> +        server_info = g_malloc0(num_servers * sizeof(*server_info));

This multiplication is (theoretically) subject to overflow. To eliminate
this risk, this should use

    g_new0(mcd_server_info_st, num_servers)

which will validate overflow & abort if hit.

There are many more instances of this code pattern in the series

$ git diff -r master | grep g_malloc | grep ' \* '
+        .tx = g_malloc(txlist->num_tx * sizeof(mcd_tx_st)),
+        server_info = g_malloc0(num_servers * sizeof(*server_info));
+        system_con_info = g_malloc0(num_systems * sizeof(*system_con_info));
+        device_con_info = g_malloc0(num_devices * sizeof(*device_con_info));
+        core_con_info = g_malloc0(num_cores * sizeof(*core_con_info));
+        memspaces = g_malloc0(num_mem_spaces * sizeof(*memspaces));
+        reg_groups = g_malloc0(num_reg_groups * sizeof(*reg_groups));
+        regs = g_malloc0(num_regs * sizeof(*regs));
+        ctrig_info = g_malloc0(num_ctrigs * sizeof(*ctrig_info));
+        trig_ids = g_malloc0(num_trigs * sizeof(*trig_ids));


QEMU is a bit inconsistent, but we have a slight bias in favour
of using g_new0, even for single struct allocations.

IMHO being in the habit of always using g_new0 instead of g_malloc
makes it less likely for people to inadvertantly introduce the
multiplication overflow code pattern with g_malloc.


With 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 :|