On Thu, 13 Apr 2023 17:33:28 +0800
Hao Zeng <zeng...@kylinos.cn> wrote:
> opened file processor not closed,May cause file processor leaks
Patch description needs to say more on how this is fixed.
Perhaps something like:
"Open file descriptor not closed in error paths. Fix by replace
open coded handling of read of whole file into a buffer with
g_file_get_contents()"
Fixes tag is part of the tag block so blank line here
> Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation")
>
An no blank line here.
> Signed-off-by: Zeng Hao <zeng...@kylinos.cn>
> Suggested-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Suggested-by: Peter Maydell <peter.mayd...@linaro.org>
>
> ---
> ChangeLog:
> v3-v4:
> Modify commit information,No code change.
> v2->v3:
> Submission of v3 on the basis of v2, based on Philippe
> Mathieu-Daudé's suggestion
> "Pointless bzero in g_malloc0, however this code would be
> simplified using g_file_get_contents()."
> v1->v2:
> - Patch 1: No change in patch v1
> - Patch 2: Fix the check on the return value of fread() in
> ct3_load_cdat
> ---
> hw/cxl/cxl-cdat.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 137abd0992..42c7c2031c 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -110,29 +110,17 @@ static void ct3_load_cdat(CDATObject *cdat, Error
> **errp)
> g_autofree CDATEntry *cdat_st = NULL;
> uint8_t sum = 0;
> int num_ent;
> - int i = 0, ent = 1, file_size = 0;
> + int i = 0, ent = 1;
> + gsize file_size = 0;
> CDATSubHeader *hdr;
> - FILE *fp = NULL;
> -
> + GError *error = NULL;
Blank line here.
> /* Read CDAT file and create its cache */
> - fp = fopen(cdat->filename, "r");
> - if (!fp) {
> - error_setg(errp, "CDAT: Unable to open file");
> - return;
> - }
> -
> - fseek(fp, 0, SEEK_END);
> - file_size = ftell(fp);
> - fseek(fp, 0, SEEK_SET);
> - cdat->buf = g_malloc0(file_size);
> -
> - if (fread(cdat->buf, file_size, 1, fp) == 0) {
> - error_setg(errp, "CDAT: File read failed");
> + if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
> + &file_size, &error)) {
Align parameters with start of 'cdat' (just after the opening bracket)
> + error_setg(errp, "CDAT: File read failed: %s", error->message);
> + g_error_free(error);
> return;
> }
> -
> - fclose(fp);
> -
> if (file_size < sizeof(CDATTableHeader)) {
> error_setg(errp, "CDAT: File too short");
> return;
> @@ -218,7 +206,5 @@ void cxl_doe_cdat_release(CXLComponentState *cxl_cstate)
> cdat->free_cdat_table(cdat->built_buf, cdat->built_buf_len,
> cdat->private);
> }
> - if (cdat->buf) {
> - free(cdat->buf);
> - }
> + g_free(cdat->buf);
Keep the protection if moving to g_free(). Not all paths to this function
allocate cdat->buf
Protection was not needed when the call was free() though.
I have a followup patch that will deal with the other issues Peter pointed out.
I'll
send that once yours has been finalized.
Thanks,
Jonathan
> }
