[PATCH v2 0/2] cxl-cdat:Fix two problems of ct3_load_cdat

[PATCH v2 0/2] cxl-cdat:Fix two problems of ct3_load_cdat

[Hao Zeng]

This v2 contains a change suggested by Peter Maydell in patch v1.
v1 link:https://lore.kernel.org/all/20230403084245.54861-1-zenghao@kylinos.cn/

ChangeLog:
    v1->v2:
    - Patch 1: No change in patch v1
    - Patch 2: Fix the check on the return value of fread() in ct3_load_cdat

Hao Zeng (2):
  cxl-cdat:Fix open file not closed in ct3_load_cdat
  cxl-cdat:Fix the check on the return value of fread()

 hw/cxl/cxl-cdat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.37.2


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH v2 1/2] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Hao Zeng]

opened file processor not closed,May cause file processor leaks
Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c

Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
Suggested-by: Xie Ming <xieming@kylinos.cn>
---
 hw/cxl/cxl-cdat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 137abd0992..ba7ed1aafd 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
 
     if (fread(cdat->buf, file_size, 1, fp) == 0) {
         error_setg(errp, "CDAT: File read failed");
+        fclose(fp);
         return;
     }
 
-- 
2.37.2


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH v2 2/2] cxl-cdat:Fix the check on the return value of fread()

[Hao Zeng]

The bug in this code (CID 1507822) is that the
check on the return value of fread() is wrong. fread()
returns the number of items read or written, so
checking for == 0 only catches "no data read at all",
not "only read half the data".

Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/cxl/cxl-cdat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index ba7ed1aafd..130531a9cd 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -126,7 +126,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     fseek(fp, 0, SEEK_SET);
     cdat->buf = g_malloc0(file_size);
 
-    if (fread(cdat->buf, file_size, 1, fp) == 0) {
+    if (fread(cdat->buf, file_size, 1, fp) != file_size) {
         error_setg(errp, "CDAT: File read failed");
         fclose(fp);
         return;
-- 
2.37.2


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH v2 1/2] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Philippe Mathieu-Daudé]

On 12/4/23 09:16, Hao Zeng wrote:
> opened file processor not closed,May cause file processor leaks
> Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c

Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
> Suggested-by: Xie Ming <xieming@kylinos.cn>
> ---
>   hw/cxl/cxl-cdat.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 137abd0992..ba7ed1aafd 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>   
>       if (fread(cdat->buf, file_size, 1, fp) == 0) {
>           error_setg(errp, "CDAT: File read failed");
> +        fclose(fp);
>           return;
>       }

Alternatively:

-- >8 --
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 137abd0992..e014b51714 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -110,7 +110,7 @@ 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, ent = 1, file_size = 0;
      CDATSubHeader *hdr;
      FILE *fp = NULL;

@@ -126,13 +126,13 @@ static void ct3_load_cdat(CDATObject *cdat, Error 
**errp)
      fseek(fp, 0, SEEK_SET);
      cdat->buf = g_malloc0(file_size);

-    if (fread(cdat->buf, file_size, 1, fp) == 0) {
+    i = fread(cdat->buf, file_size, 1, fp);
+    fclose(fp);
+    if (i == 0) {
          error_setg(errp, "CDAT: File read failed");
          return;
      }

-    fclose(fp);
-
      if (file_size < sizeof(CDATTableHeader)) {
          error_setg(errp, "CDAT: File too short");
          return;
---

Re: [PATCH v2 2/2] cxl-cdat:Fix the check on the return value of fread()

[Philippe Mathieu-Daudé]

On 12/4/23 09:16, Hao Zeng wrote:
> The bug in this code (CID 1507822) is that the
> check on the return value of fread() is wrong. fread()
> returns the number of items read or written, so
> checking for == 0 only catches "no data read at all",
> not "only read half the data".
> 
> Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/cxl/cxl-cdat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index ba7ed1aafd..130531a9cd 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -126,7 +126,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>       fseek(fp, 0, SEEK_SET);
>       cdat->buf = g_malloc0(file_size);

Pointless bzero in g_malloc0, however this code would be
simplified using g_file_get_contents().

>   
> -    if (fread(cdat->buf, file_size, 1, fp) == 0) {
> +    if (fread(cdat->buf, file_size, 1, fp) != file_size) {
>           error_setg(errp, "CDAT: File read failed");
>           fclose(fp);
>           return;

Re: [PATCH v2 2/2] cxl-cdat:Fix the check on the return value of fread()

[Jonathan Cameron via]

On Wed, 12 Apr 2023 12:02:47 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 12/4/23 09:16, Hao Zeng wrote:
> > The bug in this code (CID 1507822) is that the
> > check on the return value of fread() is wrong. fread()
> > returns the number of items read or written, so
> > checking for == 0 only catches "no data read at all",
> > not "only read half the data".
> > 
> > Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/cxl/cxl-cdat.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index ba7ed1aafd..130531a9cd 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -126,7 +126,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> >       fseek(fp, 0, SEEK_SET);
> >       cdat->buf = g_malloc0(file_size);  
> 
> Pointless bzero in g_malloc0, however this code would be
> simplified using g_file_get_contents().

Agreed - switching this whole thing to g_file_get_contents()
will get rid of this code and be a lot simpler.
Perhaps just jump directly to that and note the two bugs that existed
in the code that is replaced?

Jonathan
 

> 
> >   
> > -    if (fread(cdat->buf, file_size, 1, fp) == 0) {
> > +    if (fread(cdat->buf, file_size, 1, fp) != file_size) {
> >           error_setg(errp, "CDAT: File read failed");
> >           fclose(fp);
> >           return;  
>