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

[Hao Zeng <zenghao@kylinos.cn>]

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