[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] hw/cxl: cdat: Fix failure to free buffer in erorr paths
From: |
Fan Ni |
Subject: |
Re: [PATCH 2/2] hw/cxl: cdat: Fix failure to free buffer in erorr paths |
Date: |
Tue, 16 May 2023 16:15:23 +0000 |
On Fri, Apr 21, 2023 at 02:20:20PM +0100, Jonathan Cameron via wrote:
> The failure paths in CDAT file loading did not clear up properly.
> Change to using g_auto_free and a local pointer for the buffer to
> ensure this function has no side effects on error.
> Also drop some unnecessary checks that can not fail.
>
> Cleanup properly after a failure to load a CDAT file.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-cdat.c | 33 ++++++++++++++++++---------------
> hw/mem/cxl_type3.c | 4 ++++
> hw/pci-bridge/cxl_upstream.c | 3 +++
> 3 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 056711d63d..d246d6885b 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -108,6 +108,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> {
> g_autofree CDATEntry *cdat_st = NULL;
> + g_autofree char *buf = NULL;
> uint8_t sum = 0;
> int num_ent;
> int i = 0, ent = 1;
> @@ -116,7 +117,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> GError *error = NULL;
>
> /* Read CDAT file and create its cache */
> - if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
> + if (!g_file_get_contents(cdat->filename, (gchar **)&buf,
> &file_size, &error)) {
> error_setg(errp, "CDAT: File read failed: %s", error->message);
> g_error_free(error);
> @@ -129,9 +130,17 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> i = sizeof(CDATTableHeader);
> num_ent = 1;
> while (i < file_size) {
> - hdr = (CDATSubHeader *)(cdat->buf + i);
> + hdr = (CDATSubHeader *)(buf + i);
> + if (i + sizeof(CDATSubHeader) > file_size) {
> + error_setg(errp, "CDAT: Truncated table");
> + return;
> + }
> cdat_len_check(hdr, errp);
> i += hdr->length;
> + if (i > file_size) {
> + error_setg(errp, "CDAT: Truncated table");
> + return;
> + }
> num_ent++;
> }
> if (i != file_size) {
> @@ -139,33 +148,26 @@ static void ct3_load_cdat(CDATObject *cdat, Error
> **errp)
> return;
> }
>
> - cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);
> - if (!cdat_st) {
> - error_setg(errp, "CDAT: Failed to allocate entry array");
> - return;
> - }
> + cdat_st = g_new0(CDATEntry, num_ent);
>
> /* Set CDAT header, Entry = 0 */
> - cdat_st[0].base = cdat->buf;
> + cdat_st[0].base = buf;
> cdat_st[0].length = sizeof(CDATTableHeader);
> i = 0;
>
> while (i < cdat_st[0].length) {
> - sum += cdat->buf[i++];
> + sum += buf[i++];
> }
>
> /* Read CDAT structures */
> while (i < file_size) {
> - hdr = (CDATSubHeader *)(cdat->buf + i);
> - cdat_len_check(hdr, errp);
> -
> + hdr = (CDATSubHeader *)(buf + i);
> cdat_st[ent].base = hdr;
> cdat_st[ent].length = hdr->length;
>
> - while (cdat->buf + i <
> - (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
> + while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) {
> assert(i < file_size);
> - sum += cdat->buf[i++];
> + sum += buf[i++];
> }
>
> ent++;
> @@ -176,6 +178,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> }
> cdat->entry_len = num_ent;
> cdat->entry = g_steal_pointer(&cdat_st);
> + cdat->buf = g_steal_pointer(&buf);
> }
>
> void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index abe60b362c..7647122cc6 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -593,6 +593,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
> cxl_cstate->cdat.private = ct3d;
> cxl_doe_cdat_init(cxl_cstate, errp);
> + if (*errp) {
> + goto err_free_special_ops;
> + }
>
> pcie_cap_deverr_init(pci_dev);
> /* Leave a bit of room for expansion */
> @@ -605,6 +608,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>
> err_release_cdat:
> cxl_doe_cdat_release(cxl_cstate);
> +err_free_special_ops:
> g_free(regs->special_ops);
> err_address_space_free:
> address_space_destroy(&ct3d->hostmem_as);
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index 9df436cb73..ef47e5d625 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -346,6 +346,9 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
> cxl_cstate->cdat.free_cdat_table = free_default_cdat_table;
> cxl_cstate->cdat.private = d;
> cxl_doe_cdat_init(cxl_cstate, errp);
> + if (*errp) {
> + goto err_cap;
> + }
>
> return;
>
> --
> 2.37.2
>
>
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH 2/2] hw/cxl: cdat: Fix failure to free buffer in erorr paths,
Fan Ni <=