[PATCH 1/2] erofs-utils: complete missing memory handling

[PATCH 1/2] erofs-utils: complete missing memory handling

[Gao Xiang]

From: Li Guifu <bluce.liguifu@huawei.com>

memory allocation failure should be handled
properly in principle.

Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
[ Gao Xiang: due to Huawei outgoing email limitation,
  I have to help Guifu send out his patches at work. ]
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 lib/inode.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 86c465ee2f78..620db60f4a5b 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
 	if (used) {
 		/* fill tail-end dir block */
 		dir->idata = malloc(used);
+		if (!dir->idata)
+			return -ENOMEM;
 		DBG_BUGON(used != dir->idata_size);
 		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
 	}
@@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
+		if (!inode->idata)
+			return -ENOMEM;
 		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
 		       inode->idata_size);
 	}
@@ -347,9 +351,12 @@ int erofs_write_file(struct erofs_inode *inode)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
+		if (!inode->idata)
+			return -ENOMEM;
 
 		ret = read(fd, inode->idata, inode->idata_size);
 		if (ret < inode->idata_size) {
+			free(inode->idata);
 			close(fd);
 			return -EIO;
 		}
@@ -825,12 +832,18 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
 		if (S_ISLNK(dir->i_mode)) {
 			char *const symlink = malloc(dir->i_size);
 
+			if (!symlink)
+				return ERR_PTR(-ENOMEM);
 			ret = readlink(dir->i_srcpath, symlink, dir->i_size);
-			if (ret < 0)
+			if (ret < 0) {
+				free(symlink);
 				return ERR_PTR(-errno);
+			}
 
-			erofs_write_file_from_buffer(dir, symlink);
+			ret = erofs_write_file_from_buffer(dir, symlink);
 			free(symlink);
+			if (ret)
+				return ERR_PTR(ret);
 		} else {
 			erofs_write_file(dir);
 		}
-- 
2.17.1

[PATCH 2/2] erofs-utils: set up all compiler/linker variables independently

[Gao Xiang]

Otherwise, the following checking will be effected
and it can cause unexpected behavior on configuring.

Founded by the upcoming XZ algorithm patches.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 configure.ac | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/configure.ac b/configure.ac
index a93767f61578..a5adf172cc43 100644
--- a/configure.ac
+++ b/configure.ac
@@ -131,7 +131,7 @@ if test "x$enable_lz4" = "xyes"; then
   test -z "${with_lz4_libdir}" || LZ4_LIBS="-L$with_lz4_libdir $LZ4_LIBS"
 
   saved_CPPFLAGS=${CPPFLAGS}
-  CPPFLAGS="${LZ4_CFLAGS} ${CFLAGS}"
+  CPPFLAGS="${LZ4_CFLAGS} ${CPPFLAGS}"
 
   AC_CHECK_HEADERS([lz4.h],[have_lz4h="yes"], [])
 
@@ -150,28 +150,29 @@ if test "x$enable_lz4" = "xyes"; then
       ])
     ], [AC_MSG_ERROR([Cannot find proper lz4 version (>= 1.8.0)])])
     LDFLAGS=${saved_LDFLAGS}
-
-    if test "x${have_lz4}" = "xyes"; then
-      AC_DEFINE([LZ4_ENABLED], [1], [Define to 1 if lz4 is enabled.])
-
-      if test "x${have_lz4hc}" = "xyes"; then
-        AC_DEFINE([LZ4HC_ENABLED], [1], [Define to 1 if lz4hc is enabled.])
-      fi
-
-      if test "x${lz4_force_static}" = "xyes"; then
-        LDFLAGS="-all-static ${LDFLAGS}"
-      else
-	test -z "${with_lz4_libdir}" || LZ4_LIBS="-R ${with_lz4_libdir} $LZ4_LIBS"
-      fi
-      LIBS="$LZ4_LIBS $LIBS"
-    fi
   fi
-  CFLAGS=${saved_CPPFLAGS}
+  CPPFLAGS=${saved_CPPFLAGS}
 fi
 
+# Set up needed symbols, conditionals and compiler/linker flags
 AM_CONDITIONAL([ENABLE_LZ4], [test "x${have_lz4}" = "xyes"])
 AM_CONDITIONAL([ENABLE_LZ4HC], [test "x${have_lz4hc}" = "xyes"])
 
+if test "x${have_lz4}" = "xyes"; then
+  AC_DEFINE([LZ4_ENABLED], [1], [Define to 1 if lz4 is enabled.])
+
+  if test "x${have_lz4hc}" = "xyes"; then
+    AC_DEFINE([LZ4HC_ENABLED], [1], [Define to 1 if lz4hc is enabled.])
+  fi
+
+  if test "x${lz4_force_static}" = "xyes"; then
+    LDFLAGS="-all-static ${LDFLAGS}"
+  else
+    test -z "${with_lz4_libdir}" || LZ4_LIBS="-R ${with_lz4_libdir} $LZ4_LIBS"
+  fi
+  LIBS="$LZ4_LIBS $LIBS"
+fi
+
 AC_CONFIG_FILES([Makefile
 		 man/Makefile
 		 lib/Makefile
-- 
2.17.1

[PATCH v2] erofs-utils: complete missing memory handling

[Li Guifu]

From: Li Guifu <bluce.liguifu@huawei.com>

memory allocation failure should be handled
properly in principle.

Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
[ Gao Xiang: due to Huawei outgoing email limitation,
  I have to help Guifu send out his patches at work. ]
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
Signed-off-by: Li Guifu <blucerlee@gmail.com>
---
 lib/inode.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 86c465e..b6c2b13 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
 	if (used) {
 		/* fill tail-end dir block */
 		dir->idata = malloc(used);
+		if (!dir->idata)
+			return -ENOMEM;
 		DBG_BUGON(used != dir->idata_size);
 		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
 	}
@@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
+		if (!inode->idata)
+			return -ENOMEM;
 		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
 		       inode->idata_size);
 	}
@@ -347,9 +351,14 @@ int erofs_write_file(struct erofs_inode *inode)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
-
+		if (!inode->idata) {
+			errno = ENOMEM;
+			goto fail;
+		}
 		ret = read(fd, inode->idata, inode->idata_size);
 		if (ret < inode->idata_size) {
+			free(inode->idata);
+			inode->idata = NULL;
 			close(fd);
 			return -EIO;
 		}
@@ -825,12 +834,18 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
 		if (S_ISLNK(dir->i_mode)) {
 			char *const symlink = malloc(dir->i_size);
 
+			if (!symlink)
+				return ERR_PTR(-ENOMEM);
 			ret = readlink(dir->i_srcpath, symlink, dir->i_size);
-			if (ret < 0)
+			if (ret < 0) {
+				free(symlink);
 				return ERR_PTR(-errno);
+			}
 
-			erofs_write_file_from_buffer(dir, symlink);
+			ret = erofs_write_file_from_buffer(dir, symlink);
 			free(symlink);
+			if (ret)
+				return ERR_PTR(ret);
 		} else {
 			erofs_write_file(dir);
 		}
-- 
2.17.1

Re: [PATCH 2/2] erofs-utils: set up all compiler/linker variables independently

[Li Guifu]

Gao Xiang
	Can you resend this patch ? It is conflict with uuid.
	uuid patch :
	https://lore.kernel.org/linux-erofs/d4d8127a-c452-f7d4-b3b1-50098cf07e12@gmail.com/T/#u

On 2019/11/12 19:26, Gao Xiang wrote:
> Otherwise, the following checking will be effected
> and it can cause unexpected behavior on configuring.
> 
> Founded by the upcoming XZ algorithm patches.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---

Re: [PATCH v2] erofs-utils: complete missing memory handling

[Gao Xiang via Linux-erofs]

Hi Guifu,

On Thu, Nov 14, 2019 at 01:03:35AM +0800, Li Guifu wrote:
> From: Li Guifu <bluce.liguifu@huawei.com>
> 
> memory allocation failure should be handled
> properly in principle.
> 
> Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
> [ Gao Xiang: due to Huawei outgoing email limitation,
>   I have to help Guifu send out his patches at work. ]
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> Signed-off-by: Li Guifu <blucerlee@gmail.com>
> ---

As a common practice, It's perferred to leave some useful
comments at this about what you modified compared wtih
the last version.

>  lib/inode.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 86c465e..b6c2b13 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
>  	if (used) {
>  		/* fill tail-end dir block */
>  		dir->idata = malloc(used);
> +		if (!dir->idata)
> +			return -ENOMEM;
>  		DBG_BUGON(used != dir->idata_size);
>  		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
>  	}
> @@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
>  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
>  	if (inode->idata_size) {
>  		inode->idata = malloc(inode->idata_size);
> +		if (!inode->idata)
> +			return -ENOMEM;
>  		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
>  		       inode->idata_size);
>  	}
> @@ -347,9 +351,14 @@ int erofs_write_file(struct erofs_inode *inode)
>  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
>  	if (inode->idata_size) {
>  		inode->idata = malloc(inode->idata_size);
> -
> +		if (!inode->idata) {
> +			errno = ENOMEM;
> +			goto fail;
> +		}
>  		ret = read(fd, inode->idata, inode->idata_size);
>  		if (ret < inode->idata_size) {
> +			free(inode->idata);
> +			inode->idata = NULL;

Anyway, it seems the diffstat is this line.
I think it' better than v1 so let's use this version.

Thanks,
Gao Xiang

Re: [PATCH 2/2] erofs-utils: set up all compiler/linker variables independently

[Gao Xiang via Linux-erofs]

Hi Guifu,

On Thu, Nov 14, 2019 at 01:12:26AM +0800, Li Guifu wrote:
> Gao Xiang
> 	Can you resend this patch ? It is conflict with uuid.
> 	uuid patch :
> 	https://lore.kernel.org/linux-erofs/d4d8127a-c452-f7d4-b3b1-50098cf07e12@gmail.com/T/#u

You're right. I'll sort out all erofs-utils pending patches,
rebase this patch and resend soon.

Thanks,
Gao Xiang

> 
> On 2019/11/12 19:26, Gao Xiang wrote:
> > Otherwise, the following checking will be effected
> > and it can cause unexpected behavior on configuring.
> > 
> > Founded by the upcoming XZ algorithm patches.
> > 
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---

Re: [PATCH v2] erofs-utils: complete missing memory handling

[Gao Xiang]

On Thu, Nov 14, 2019 at 08:24:31AM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Guifu,
> 
> On Thu, Nov 14, 2019 at 01:03:35AM +0800, Li Guifu wrote:
> > From: Li Guifu <bluce.liguifu@huawei.com>
> > 
> > memory allocation failure should be handled
> > properly in principle.
> > 
> > Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
> > [ Gao Xiang: due to Huawei outgoing email limitation,
> >   I have to help Guifu send out his patches at work. ]
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > Signed-off-by: Li Guifu <blucerlee@gmail.com>
> > ---
> 
> As a common practice, It's perferred to leave some useful
> comments at this about what you modified compared wtih
> the last version.
> 
> >  lib/inode.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 86c465e..b6c2b13 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
> >  	if (used) {
> >  		/* fill tail-end dir block */
> >  		dir->idata = malloc(used);
> > +		if (!dir->idata)
> > +			return -ENOMEM;
> >  		DBG_BUGON(used != dir->idata_size);
> >  		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
> >  	}
> > @@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
> >  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
> >  	if (inode->idata_size) {
> >  		inode->idata = malloc(inode->idata_size);
> > +		if (!inode->idata)
> > +			return -ENOMEM;
> >  		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
> >  		       inode->idata_size);
> >  	}
> > @@ -347,9 +351,14 @@ int erofs_write_file(struct erofs_inode *inode)
> >  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
> >  	if (inode->idata_size) {
> >  		inode->idata = malloc(inode->idata_size);
> > -
> > +		if (!inode->idata) {
> > +			errno = ENOMEM;
> > +			goto fail;
> > +		}

When I revisited this patch, I noticed it's some weird to operate `errno' here.

The same sequence "close(fd); return -ENOMEM;" is indeed some unclean, but
I think you could make a separate cleanup patch then.

I will resend PATCH v3 since you aren't able to post public mail at work
and apply it to experimental branch.

Thanks,
Gao Xiang

> >  		ret = read(fd, inode->idata, inode->idata_size);
> >  		if (ret < inode->idata_size) {
> > +			free(inode->idata);
> > +			inode->idata = NULL;
> 
> Anyway, it seems the diffstat is this line.
> I think it' better than v1 so let's use this version.
> 
> Thanks,
> Gao Xiang
> 

[PATCH v3] erofs-utils: complete missing memory handling

[Gao Xiang via Linux-erofs]

From: Li Guifu <bluce.liguifu@huawei.com>

Memory allocation failure should be handled
properly in principle.

Link: https://lore.kernel.org/r/20191113170335.17621-1-blucerlee@gmail.com
Signed-off-by: Li Guifu <bluce.liguifu@huawei.com>
[ Gao Xiang: due to Huawei outgoing email limitation,
  I have to help Guifu send out his patches at work. ]
Signed-off-by: Li Guifu <blucerlee@gmail.com>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
changes since v2:
 - avoid using system errno mechanism to report errors:
   https://lore.kernel.org/r/20191114015110.GA155186@architecture4

 lib/inode.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 86c465e..0e19b11 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -264,6 +264,8 @@ int erofs_write_dir_file(struct erofs_inode *dir)
 	if (used) {
 		/* fill tail-end dir block */
 		dir->idata = malloc(used);
+		if (!dir->idata)
+			return -ENOMEM;
 		DBG_BUGON(used != dir->idata_size);
 		fill_dirblock(dir->idata, dir->idata_size, q, head, d);
 	}
@@ -286,6 +288,8 @@ int erofs_write_file_from_buffer(struct erofs_inode *inode, char *buf)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
+		if (!inode->idata)
+			return -ENOMEM;
 		memcpy(inode->idata, buf + blknr_to_addr(nblocks),
 		       inode->idata_size);
 	}
@@ -347,9 +351,15 @@ int erofs_write_file(struct erofs_inode *inode)
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
+		if (!inode->idata) {
+			close(fd);
+			return -ENOMEM;
+		}
 
 		ret = read(fd, inode->idata, inode->idata_size);
 		if (ret < inode->idata_size) {
+			free(inode->idata);
+			inode->idata = NULL;
 			close(fd);
 			return -EIO;
 		}
@@ -825,12 +835,18 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
 		if (S_ISLNK(dir->i_mode)) {
 			char *const symlink = malloc(dir->i_size);
 
+			if (!symlink)
+				return ERR_PTR(-ENOMEM);
 			ret = readlink(dir->i_srcpath, symlink, dir->i_size);
-			if (ret < 0)
+			if (ret < 0) {
+				free(symlink);
 				return ERR_PTR(-errno);
+			}
 
-			erofs_write_file_from_buffer(dir, symlink);
+			ret = erofs_write_file_from_buffer(dir, symlink);
 			free(symlink);
+			if (ret)
+				return ERR_PTR(ret);
 		} else {
 			erofs_write_file(dir);
 		}
-- 
2.17.1

[PATCH v2] erofs-utils: set up all compiler/linker variables independently

[Gao Xiang via Linux-erofs]

Otherwise, the following checking will be effected
and it can cause unexpected behavior on configuring.

Founded by the upcoming XZ algorithm patches.

Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
changes since v1:
 - rebase the patch suggested by Guifu.

 configure.ac | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/configure.ac b/configure.ac
index c7dbe0e..f925358 100644
--- a/configure.ac
+++ b/configure.ac
@@ -168,7 +168,7 @@ if test "x$enable_lz4" = "xyes"; then
   test -z "${with_lz4_libdir}" || LZ4_LIBS="-L$with_lz4_libdir $LZ4_LIBS"
 
   saved_CPPFLAGS=${CPPFLAGS}
-  CPPFLAGS="${LZ4_CFLAGS} ${CFLAGS}"
+  CPPFLAGS="${LZ4_CFLAGS} ${CPPFLAGS}"
 
   AC_CHECK_HEADERS([lz4.h],[have_lz4h="yes"], [])
 
@@ -187,31 +187,32 @@ if test "x$enable_lz4" = "xyes"; then
       ])
     ], [AC_MSG_ERROR([Cannot find proper lz4 version (>= 1.8.0)])])
     LDFLAGS=${saved_LDFLAGS}
-
-    if test "x${have_lz4}" = "xyes"; then
-      AC_DEFINE([LZ4_ENABLED], [1], [Define to 1 if lz4 is enabled.])
-
-      if test "x${have_lz4hc}" = "xyes"; then
-        AC_DEFINE([LZ4HC_ENABLED], [1], [Define to 1 if lz4hc is enabled.])
-      fi
-
-      if test "x${lz4_force_static}" = "xyes"; then
-        LDFLAGS="-all-static ${LDFLAGS}"
-      else
-	test -z "${with_lz4_libdir}" || LZ4_LIBS="-R ${with_lz4_libdir} $LZ4_LIBS"
-      fi
-      LIBS="$LZ4_LIBS $LIBS"
-    fi
   fi
-  CFLAGS=${saved_CPPFLAGS}
+  CPPFLAGS=${saved_CPPFLAGS}
 fi
 
+# Set up needed symbols, conditionals and compiler/linker flags
+AM_CONDITIONAL([ENABLE_LZ4], [test "x${have_lz4}" = "xyes"])
+AM_CONDITIONAL([ENABLE_LZ4HC], [test "x${have_lz4hc}" = "xyes"])
+
 if test "x$have_uuid" = "xyes"; then
   AC_DEFINE([HAVE_LIBUUID], 1, [Define to 1 if libuuid is found])
 fi
 
-AM_CONDITIONAL([ENABLE_LZ4], [test "x${have_lz4}" = "xyes"])
-AM_CONDITIONAL([ENABLE_LZ4HC], [test "x${have_lz4hc}" = "xyes"])
+if test "x${have_lz4}" = "xyes"; then
+  AC_DEFINE([LZ4_ENABLED], [1], [Define to 1 if lz4 is enabled.])
+
+  if test "x${have_lz4hc}" = "xyes"; then
+    AC_DEFINE([LZ4HC_ENABLED], [1], [Define to 1 if lz4hc is enabled.])
+  fi
+
+  if test "x${lz4_force_static}" = "xyes"; then
+    LDFLAGS="-all-static ${LDFLAGS}"
+  else
+    test -z "${with_lz4_libdir}" || LZ4_LIBS="-R ${with_lz4_libdir} $LZ4_LIBS"
+  fi
+  LIBS="$LZ4_LIBS $LIBS"
+fi
 
 AC_CONFIG_FILES([Makefile
 		 man/Makefile
-- 
2.17.1

Re: [PATCH v2] erofs-utils: set up all compiler/linker variables independently

[Li Guifu]

On 2019/11/14 21:45, Gao Xiang wrote:
> Otherwise, the following checking will be effected
> and it can cause unexpected behavior on configuring.
> 
> Founded by the upcoming XZ algorithm patches.
> 
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---

It looks good
Reviewed-by: Li Guifu <blucerlee@gmail.com>
Tested-by: Li Guifu <blucerlee@gmail.com>

Thanks,