Re: [PULL v4 25/83] hw/pci-bridge/cxl-upstream: Add a CDAT table access DOE

[Peter Maydell]

On Mon, 7 Nov 2022 at 22:49, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This Data Object Exchange Mailbox allows software to query the
> latency and bandwidth between ports on the switch. For now
> only provide information on routes between the upstream port and
> each downstream port (not p2p).
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Hi; Coverity points out (CID 1508120) something a bit odd
in this code:

> +static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> +{
> +    g_autofree CDATSslbis *sslbis_latency = NULL;
> +    g_autofree CDATSslbis *sslbis_bandwidth = NULL;
> +    CXLUpstreamPort *us = CXL_USP(priv);
> +    PCIBus *bus = &PCI_BRIDGE(us)->sec_bus;
> +    int devfn, sslbis_size, i;
> +    int count = 0;
> +    uint16_t port_ids[256];

> +    *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES);

Here:
 - cdat_table has type CDATSubHeader ***
 - so *cdat_table has type CDATSubHeader **
 - so the array we're allocating here should be items of type CDATSubHeader *
 - but we pass sizeof(*cdat_table), which is sizeof(CDATSubHeader **),
   implying that we're allocating an array of CDATSubHeader **

It happens that sizeof(CDATSubHeader **) == sizeof(CDATSubHeader *)
so nothing blows up, but presumably this was supposed to be
sizeof(**cdat_table) ?

Using g_new0() lets you pass in exactly the type you're creating
an array of and avoids having to do the multiplication explicitly.
It also gets the type checking right because the return type
from g_new0(my_type, N) is "my_type *".

  *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES);

As a side observation, "Foo ***" is a really awkward type
to be working with and it's not surprising that this kind
of "did I mean to dereference once, twice or three times?"
bug has crept in. Perhaps there's some refactoring that
could avoid that ***...

> +    if (!*cdat_table) {
> +        return -ENOMEM;
> +    }

Also g_malloc0() and g_new0() can never fail, so don't check
them for failure.

thanks
-- PMM