[PATCH v4] cxl-cdat:Fix open file not closed in ct3_load_cdat

[PATCH v4] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Hao Zeng]

[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]


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(-)

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);
 }
-- 
2.37.2


[-- Attachment #2: Type: text/plain, Size: 82 bytes --]

Content-type: Text/plain

No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH v4] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Jonathan Cameron via]

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



>  }

Re: [PATCH v4] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Hao Zeng]

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

Re: [PATCH v4] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Hao Zeng]

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