| Message ID | 20230413093328.3689564-1-zenghao@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] cxl-cdat:Fix open file not closed in ct3_load_cdat | expand |
On Thu, 2023-04-13 at 12:17 +0100, Jonathan Cameron wrote: > On Thu, 13 Apr 2023 17:33:28 +0800 > Hao Zeng <zenghao@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 <zenghao@kylinos.cn> > > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Suggested-by: Peter Maydell <peter.maydell@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 > > > > > } > Dear Jonathan Thank you for taking the time to reply to my email. I appreciate your the valuable information you have provided. Already submitted in v5 according to the modifications. Best regards Hao
On Thu, 2023-04-13 at 12:17 +0100, Jonathan Cameron wrote: > On Thu, 13 Apr 2023 17:33:28 +0800 > Hao Zeng <zenghao@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 <zenghao@kylinos.cn> > > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Suggested-by: Peter Maydell <peter.maydell@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 > > > > > } > Dear Jonathan Thank you for taking the time to reply to my email. I appreciate your the valuable information you have provided. Already submitted in v5 according to the modifications. Best regards Hao
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; /* 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)) { + 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); }
opened file processor not closed,May cause file processor leaks Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation") Signed-off-by: Zeng Hao <zenghao@kylinos.cn> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> Suggested-by: Peter Maydell <peter.maydell@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(-)