hw/cxl/cxl-cdat.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)
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: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation")
Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
Suggested-by: Philippe Mathieu-Daud\ufffd\ufffd <philmd@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Jonathan Cameron via <qemu-devel@nongnu.org>
---
ChangeLog:
v4-v5:
fixes some style issues and keep the protection after using g_free()
v3-v4:
Modify commit information,No code change.
v2->v3:
Submission of v3 on the basis of v2, based on Philippe Mathieu-Daud\ufffd\ufffd'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 | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 137abd0992..dd69366797 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -110,29 +110,18 @@ 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");
+ 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;
}
-
- 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");
- return;
- }
-
- fclose(fp);
-
if (file_size < sizeof(CDATTableHeader)) {
error_setg(errp, "CDAT: File too short");
return;
@@ -219,6 +208,6 @@ void cxl_doe_cdat_release(CXLComponentState *cxl_cstate)
cdat->private);
}
if (cdat->buf) {
- free(cdat->buf);
+ g_free(cdat->buf);
}
}
--
2.37.2
Content-type: Text/plain
No virus found
Checked by Hillstone Network AntiVirus
On Thu, 13 Apr 2023 20:23:58 +0800 Hao Zeng <zenghao@kylinos.cn> wrote: > 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: 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> > Suggested-by: Jonathan Cameron via <qemu-devel@nongnu.org> > > --- > ChangeLog: > v4-v5: > fixes some style issues and keep the protection after using g_free() > 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 | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c > index 137abd0992..dd69366797 100644 > --- a/hw/cxl/cxl-cdat.c > +++ b/hw/cxl/cxl-cdat.c > @@ -110,29 +110,18 @@ 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"); > + 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; > } > - > - 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"); > - return; > - } > - > - fclose(fp); > - > if (file_size < sizeof(CDATTableHeader)) { > error_setg(errp, "CDAT: File too short"); > return; > @@ -219,6 +208,6 @@ void cxl_doe_cdat_release(CXLComponentState *cxl_cstate) > cdat->private); > } > if (cdat->buf) { Check patch complains about this check being unnecessary. I'll drop the check and then pick up this patch as a precusor to the other stuff Peter pointed out in this area. Thanks, Jonathan > - free(cdat->buf); > + g_free(cdat->buf); > } > }
hi Jonathan: Thank you very much Best regards --- Hao On Fri, 2023-04-21 at 14:14 +0100, Jonathan Cameron wrote: > On Thu, 13 Apr 2023 20:23:58 +0800 > Hao Zeng <zenghao@kylinos.cn> wrote: > > > 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: 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> > > Suggested-by: Jonathan Cameron via <qemu-devel@nongnu.org> > > > > --- > > ChangeLog: > > v4-v5: > > fixes some style issues and keep the protection after using > > g_free() > > 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 | 27 ++++++++------------------- > > 1 file changed, 8 insertions(+), 19 deletions(-) > > > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c > > index 137abd0992..dd69366797 100644 > > --- a/hw/cxl/cxl-cdat.c > > +++ b/hw/cxl/cxl-cdat.c > > @@ -110,29 +110,18 @@ 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"); > > + 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; > > } > > - > > - 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"); > > - return; > > - } > > - > > - fclose(fp); > > - > > if (file_size < sizeof(CDATTableHeader)) { > > error_setg(errp, "CDAT: File too short"); > > return; > > @@ -219,6 +208,6 @@ void cxl_doe_cdat_release(CXLComponentState > > *cxl_cstate) > > cdat->private); > > } > > if (cdat->buf) { > > Check patch complains about this check being unnecessary. I'll drop > the check > and then pick up this patch as a precusor to the other stuff Peter > pointed out in this > area. > > Thanks, > > Jonathan > > > > - free(cdat->buf); > > + g_free(cdat->buf); > > } > > } >
© 2016 - 2024 Red Hat, Inc.