[PATCH 1/3] erofs-utils: fsck: fix fd leak on failure in erofs_extract_file()

[PATCH 1/3] erofs-utils: fsck: fix fd leak on failure in erofs_extract_file()

[Gao Xiang]

Ignore the return values as other close()s instead.

Coverity-id: 502331
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fsck/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fsck/main.c b/fsck/main.c
index fb66967..bbef645 100644
--- a/fsck/main.c
+++ b/fsck/main.c
@@ -702,11 +702,9 @@ again:
 
 	/* verify data chunk layout */
 	ret = erofs_verify_inode_data(inode, fd);
+	close(fd);
 	if (ret)
 		return ret;
-
-	if (close(fd))
-		return -errno;
 	return ret;
 }
 
-- 
2.43.5

[PATCH 2/3] erofs-utils: lib: fix out-of-bounds in erofs_io_xcopy()

[Gao Xiang]

Coverity-id: 502334
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 include/erofs/io.h | 4 ++--
 lib/io.c           | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/erofs/io.h b/include/erofs/io.h
index f53abed..d9b33d2 100644
--- a/include/erofs/io.h
+++ b/include/erofs/io.h
@@ -34,7 +34,7 @@ struct erofs_vfops {
 	off_t (*lseek)(struct erofs_vfile *vf, u64 offset, int whence);
 	int (*fstat)(struct erofs_vfile *vf, struct stat *buf);
 	int (*xcopy)(struct erofs_vfile *vout, off_t pos,
-		     struct erofs_vfile *vin, int len, bool noseek);
+		     struct erofs_vfile *vin, unsigned int len, bool noseek);
 };
 
 /* don't extend this; instead, use payload for any extra information */
@@ -61,7 +61,7 @@ off_t erofs_io_lseek(struct erofs_vfile *vf, u64 offset, int whence);
 ssize_t erofs_copy_file_range(int fd_in, u64 *off_in, int fd_out, u64 *off_out,
 			      size_t length);
 int erofs_io_xcopy(struct erofs_vfile *vout, off_t pos,
-		   struct erofs_vfile *vin, int len, bool noseek);
+		   struct erofs_vfile *vin, unsigned int len, bool noseek);
 
 #ifdef __cplusplus
 }
diff --git a/lib/io.c b/lib/io.c
index 9167321..4937db5 100644
--- a/lib/io.c
+++ b/lib/io.c
@@ -490,7 +490,7 @@ off_t erofs_io_lseek(struct erofs_vfile *vf, u64 offset, int whence)
 }
 
 int erofs_io_xcopy(struct erofs_vfile *vout, off_t pos,
-		   struct erofs_vfile *vin, int len, bool noseek)
+		   struct erofs_vfile *vin, unsigned int len, bool noseek)
 {
 	if (vout->ops)
 		return vout->ops->xcopy(vout, pos, vin, len, noseek);
@@ -519,7 +519,7 @@ int erofs_io_xcopy(struct erofs_vfile *vout, off_t pos,
 
 	do {
 		char buf[32768];
-		int ret = min_t(int, len, sizeof(buf));
+		int ret = min_t(unsigned int, len, sizeof(buf));
 
 		ret = erofs_io_read(vin, buf, ret);
 		if (ret < 0)
-- 
2.43.5

Re: [PATCH 2/3] erofs-utils: lib: fix out-of-bounds in erofs_io_xcopy()

[Sandeep Dhavale via Linux-erofs]

Reviewed-by: Sandeep Dhavale <dhavale@google.com>

Thanks,
Sandeep.

[PATCH 3/3] erofs-utils: lib: fix fd leak on failure in erofs_dev_open()

[Gao Xiang]

Coverity-id: 502356
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 lib/io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/io.c b/lib/io.c
index 4937db5..6bfae69 100644
--- a/lib/io.c
+++ b/lib/io.c
@@ -258,8 +258,10 @@ repeat:
 #if defined(HAVE_SYS_STATFS_H) && defined(HAVE_FSTATFS)
 			struct statfs stfs;
 
-			if (again)
+			if (again) {
+				close(fd);
 				return -ENOTEMPTY;
+			}
 
 			/*
 			 * fses like EXT4 and BTRFS will flush dirty blocks
-- 
2.43.5

Re: [PATCH 3/3] erofs-utils: lib: fix fd leak on failure in erofs_dev_open()

[Sandeep Dhavale via Linux-erofs]

Reviewed-by: Sandeep Dhavale <dhavale@google.com>

Thanks,
Sandeep.

[PATCH 4/3] erofs-utils: lib: fix potential memory leak in erofs_export_xattr_ibody()

[Gao Xiang]

Although it won't happen in reality except for bugs.

Fixes: 8f93c2f83962 ("erofs-utils: mkfs: support inline xattr reservation for rootdirs")
Coverity-id: 507395
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 lib/xattr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/xattr.c b/lib/xattr.c
index f860f2e..651657f 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -1020,6 +1020,7 @@ char *erofs_export_xattr_ibody(struct erofs_inode *inode)
 		memset(buf + p, 0, size - p);
 	} else if (__erofs_unlikely(p > size)) {
 		DBG_BUGON(1);
+		free(buf);
 		return ERR_PTR(-EFAULT);
 	}
 	return buf;
-- 
2.43.5

Re: [PATCH 4/3] erofs-utils: lib: fix potential memory leak in erofs_export_xattr_ibody()

[Sandeep Dhavale via Linux-erofs]

Reviewed-by: Sandeep Dhavale <dhavale@google.com>

Thanks,
Sandeep.

Re: [PATCH 1/3] erofs-utils: fsck: fix fd leak on failure in erofs_extract_file()

[Sandeep Dhavale via Linux-erofs]

Hi Gao,

On Thu, Aug 1, 2024 at 6:55 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
> Ignore the return values as other close()s instead.
>
> Coverity-id: 502331
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
>  fsck/main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fsck/main.c b/fsck/main.c
> index fb66967..bbef645 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -702,11 +702,9 @@ again:
>
>         /* verify data chunk layout */
>         ret = erofs_verify_inode_data(inode, fd);
> +       close(fd);
>         if (ret)
>                 return ret;
I think we can get rid of this if block and should be just
return ret;
> -
> -       if (close(fd))
> -               return -errno;
>         return ret;
>  }
>
> --
> 2.43.5
>
You can just do that while applying,

Reviewed-by: Sandeep Dhavale <dhavale@google.com>

Thanks,
Sandeep.

Re: [PATCH 1/3] erofs-utils: fsck: fix fd leak on failure in erofs_extract_file()

[Gao Xiang]

On Fri, Aug 02, 2024 at 02:17:12PM -0700, Sandeep Dhavale via Linux-erofs wrote:
> Hi Gao,
> 
> On Thu, Aug 1, 2024 at 6:55 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >
> > Ignore the return values as other close()s instead.
> >
> > Coverity-id: 502331
> > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > ---
> >  fsck/main.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fsck/main.c b/fsck/main.c
> > index fb66967..bbef645 100644
> > --- a/fsck/main.c
> > +++ b/fsck/main.c
> > @@ -702,11 +702,9 @@ again:
> >
> >         /* verify data chunk layout */
> >         ret = erofs_verify_inode_data(inode, fd);
> > +       close(fd);
> >         if (ret)
> >                 return ret;
> I think we can get rid of this if block and should be just
> return ret;

Yeah, agreed, let me revise that.

> > -
> > -       if (close(fd))
> > -               return -errno;
> >         return ret;
> >  }
> >
> > --
> > 2.43.5
> >
> You can just do that while applying,
> 
> Reviewed-by: Sandeep Dhavale <dhavale@google.com>
> 

Thanks for your review!

Thanks,
Gao Xiang

> Thanks,
> Sandeep.