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

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

Posted by zenghao 1 year, 1 month ago

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

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

Posted by Peter Maydell 1 year, 1 month ago

On Mon, 3 Apr 2023 at 13:51, zenghao <zenghao@kylinos.cn> wrote:
>
> 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;
>      }

Coverity also spotted this, as CID 1508069.

The other 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". This check should be
 if (fread(cdat->buf, file_size, 1, fp) != file_size) {
    ...
 }
I think.

thanks
-- PMM

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

Posted by Hao Zeng 1 year, 1 month ago

On Tue, 2023-04-11 at 16:39 +0100, Peter Maydell wrote:
Dear Peter
Thank you for taking the time to reply to my email. I appreciate your
the valuable information you have provided.

> On Mon, 3 Apr 2023 at 13:51, zenghao <zenghao@kylinos.cn> wrote:
> > 
> > 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;
> >      }
> 
> Coverity also spotted this, as CID 1508069.
> 
> The other 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". This check should be
>  if (fread(cdat->buf, file_size, 1, fp) != file_size) {
>     ...
>  }
> I think.
>  
> thanks
> -- PMM
I couldn't agree more with your thoughts.
I will fix the bug in a separate commit.(CID 1507822)

Best regards
Hao

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

Posted by Fan Ni 1 year, 1 month ago

On Mon, Apr 03, 2023 at 04:42:45PM +0800, zenghao wrote:
> 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;
>      }
>  
Good catch.

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> -- 
> 2.37.2
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus