| Message ID | 20230718101327.1111374-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-8.1] hw/pci-bridge/cxl_upstream.c: Use g_new0() in build_cdat_table() | expand |
On 18/7/23 12:13, Peter Maydell wrote: > In build_cdat_table() we do: > *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); > This is wrong because: > - cdat_table has type CDATSubHeader *** Yes > - so *cdat_table has type CDATSubHeader ** Yes > - so the array we're allocating here should be items of type CDATSubHeader * Yes > - but we pass sizeof(*cdat_table), which is sizeof(CDATSubHeader **), Indeed > implying that we're allocating an array of CDATSubHeader ** Ouch > It happens that sizeof(CDATSubHeader **) == sizeof(CDATSubHeader *) Ah! > so nothing blows up, but this should be sizeof(**cdat_table). Still, what a mess :) > Avoid this excessively hard-to-understand code by using > g_new0() instead, which will do the type checking for us. > While we're here, we can drop the useless check against failure, > as g_malloc0() and g_new0() never fail. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > This fixes Coverity issue CID 1508120. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Disclaimer: I have not tested this beyond any testing you > get from 'make check' and 'make check-avocado'. > --- > hw/pci-bridge/cxl_upstream.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-)
On Tue, 18 Jul 2023 14:10:24 +0200 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 18/7/23 12:13, Peter Maydell wrote: > > In build_cdat_table() we do: > > *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); > > This is wrong because: > > - cdat_table has type CDATSubHeader *** > > Yes > > > - so *cdat_table has type CDATSubHeader ** > > Yes > > > - so the array we're allocating here should be items of type CDATSubHeader * > > Yes > > > - but we pass sizeof(*cdat_table), which is sizeof(CDATSubHeader **), > > Indeed > > > implying that we're allocating an array of CDATSubHeader ** > > Ouch > > > It happens that sizeof(CDATSubHeader **) == sizeof(CDATSubHeader *) > > Ah! > > > so nothing blows up, but this should be sizeof(**cdat_table). > > Still, what a mess :) > > > Avoid this excessively hard-to-understand code by using > > g_new0() instead, which will do the type checking for us. > > While we're here, we can drop the useless check against failure, > > as g_malloc0() and g_new0() never fail. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > This fixes Coverity issue CID 1508120. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > Disclaimer: I have not tested this beyond any testing you > > get from 'make check' and 'make check-avocado'. Ah. Had this on my todo list form your report but got distracted. Anyhow, thanks for fixing it up. Tested somewhat. Actually poking this particular DOE instance to read the CDAT requires a bunch of kernel hacks I haven't forward ported recently and I don't want to delay this until I get back to them - depends on some core PCI subsystem work that is ongoing. Code isn't dependent on reading though so I can't see how this would break anything. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > hw/pci-bridge/cxl_upstream.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) >
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c index ef47e5d6253..9159f48a8cb 100644 --- a/hw/pci-bridge/cxl_upstream.c +++ b/hw/pci-bridge/cxl_upstream.c @@ -274,10 +274,7 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv) }; } - *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); - if (!*cdat_table) { - return -ENOMEM; - } + *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES); /* Header always at start of structure */ (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = g_steal_pointer(&sslbis_latency);
In build_cdat_table() we do: *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); This is wrong because: - 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 this should be sizeof(**cdat_table). Avoid this excessively hard-to-understand code by using g_new0() instead, which will do the type checking for us. While we're here, we can drop the useless check against failure, as g_malloc0() and g_new0() never fail. This fixes Coverity issue CID 1508120. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Disclaimer: I have not tested this beyond any testing you get from 'make check' and 'make check-avocado'. --- hw/pci-bridge/cxl_upstream.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)