[PATCH 0/3] xfsprogs: avoid to use NULL pointer

[PATCH 0/3] xfsprogs: avoid to use NULL pointer

[zhanchengbin]

Change malloc to xmalloc for avoid to use NULL pointer.

zhanchengbin (3):
   mkfs/proto.c: avoid to use NULL pointer
   db: avoid to use NULL pointer
   repair/slab.c: avoid to use NULL pointer

  db/block.c    | 2 +-
  db/check.c    | 4 ++--
  db/faddr.c    | 6 +++---
  db/namei.c    | 2 +-
  mkfs/proto.c  | 4 ++--
  repair/slab.c | 2 +-
  6 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.27.0

[PATCH 1/3] mkfs/proto.c: avoid to use NULL pointer

[zhanchengbin]

Change malloc to xmalloc.

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
---
  mkfs/proto.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 127d87dd..f3b8710c 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -70,7 +70,7 @@ setup_proto(
  		goto out_fail;
  	}

-	buf = malloc(size + 1);
+	buf = xmalloc(size + 1);
  	if (read(fd, buf, size) < size) {
  		fprintf(stderr, _("%s: read failed on %s: %s\n"),
  			progname, fname, strerror(errno));
@@ -303,7 +303,7 @@ newregfile(
  		exit(1);
  	}
  	if ((*len = (int)size)) {
-		buf = malloc(size);
+		buf = xmalloc(size);
  		if (read(fd, buf, size) < size) {
  			fprintf(stderr, _("%s: read failed on %s: %s\n"),
  				progname, fname, strerror(errno));
-- 
2.27.0

Re: [PATCH 1/3] mkfs/proto.c: avoid to use NULL pointer

[Eric Sandeen]

On 6/6/22 7:33 AM, zhanchengbin wrote:
> Change malloc to xmalloc.

The commit log and cover letter say nothing about this, but apparently
"xmalloc" is often defined locally to abort() on a memory failure, so I
guess you are trying to make (some of?) xfsprogs do that.

First, this doesn't seem to build....

Building mkfs
    [CC]     proto.o
proto.c: In function ‘setup_proto’:
proto.c:73:15: warning: implicit declaration of function ‘xmalloc’; did you mean ‘malloc’? [-Wimplicit-function-declaration]
   73 |         buf = xmalloc(size + 1);
      |               ^~~~~~~
      |               malloc
proto.c:73:13: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
   73 |         buf = xmalloc(size + 1);
      |             ^
proto.c: In function ‘newregfile’:
proto.c:306:21: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  306 |                 buf = xmalloc(size);
      |                     ^
    [LD]     mkfs.xfs
/usr/bin/ld: proto.o: in function `setup_proto':
/src/git/xfsprogs-dev/mkfs/proto.c:73: undefined reference to `xmalloc'
/usr/bin/ld: proto.o: in function `newregfile':
/src/git/xfsprogs-dev/mkfs/proto.c:306: undefined reference to `xmalloc'
collect2: error: ld returned 1 exit status
gmake[2]: *** [../include/buildrules:65: mkfs.xfs] Error 1
gmake[1]: *** [include/buildrules:36: mkfs] Error 2
make: *** [Makefile:92: default] Error 2

Second, we have calls to malloc all over the codebase, including around
100 outside of the internal libraries in the various userspace utilities.
Why change only mkfs/db/repair?

Third, what problem are you solving? Granted, we should be checking every
malloc call, and it seems that we don't. But have you ever seen these
failures in practice? Your commit log should at least explain why this
makes the code better (and, I guess, where xmalloc is supposed to come 
from...)

-Eric

> Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> ---
>  mkfs/proto.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 127d87dd..f3b8710c 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -70,7 +70,7 @@ setup_proto(
>          goto out_fail;
>      }
> 
> -    buf = malloc(size + 1);
> +    buf = xmalloc(size + 1);
>      if (read(fd, buf, size) < size) {
>          fprintf(stderr, _("%s: read failed on %s: %s\n"),
>              progname, fname, strerror(errno));
> @@ -303,7 +303,7 @@ newregfile(
>          exit(1);
>      }
>      if ((*len = (int)size)) {
> -        buf = malloc(size);
> +        buf = xmalloc(size);
>          if (read(fd, buf, size) < size) {
>              fprintf(stderr, _("%s: read failed on %s: %s\n"),
>                  progname, fname, strerror(errno));

Re: [PATCH 1/3] mkfs/proto.c: avoid to use NULL pointer

[Dave Chinner]

On Mon, Jun 06, 2022 at 09:29:27AM -0500, Eric Sandeen wrote:
> On 6/6/22 7:33 AM, zhanchengbin wrote:
> > Change malloc to xmalloc.
> 
> The commit log and cover letter say nothing about this, but apparently
> "xmalloc" is often defined locally to abort() on a memory failure, so I
> guess you are trying to make (some of?) xfsprogs do that.
> 
> First, this doesn't seem to build....
> 
> Building mkfs
>     [CC]     proto.o
> proto.c: In function ‘setup_proto’:
> proto.c:73:15: warning: implicit declaration of function ‘xmalloc’; did you mean ‘malloc’? [-Wimplicit-function-declaration]
>    73 |         buf = xmalloc(size + 1);
>       |               ^~~~~~~
>       |               malloc
> proto.c:73:13: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
>    73 |         buf = xmalloc(size + 1);
>       |             ^
> proto.c: In function ‘newregfile’:
> proto.c:306:21: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
>   306 |                 buf = xmalloc(size);
>       |                     ^
>     [LD]     mkfs.xfs
> /usr/bin/ld: proto.o: in function `setup_proto':
> /src/git/xfsprogs-dev/mkfs/proto.c:73: undefined reference to `xmalloc'
> /usr/bin/ld: proto.o: in function `newregfile':
> /src/git/xfsprogs-dev/mkfs/proto.c:306: undefined reference to `xmalloc'
> collect2: error: ld returned 1 exit status
> gmake[2]: *** [../include/buildrules:65: mkfs.xfs] Error 1
> gmake[1]: *** [include/buildrules:36: mkfs] Error 2
> make: *** [Makefile:92: default] Error 2
> 
> Second, we have calls to malloc all over the codebase, including around
> 100 outside of the internal libraries in the various userspace utilities.
> Why change only mkfs/db/repair?

xmalloc is local to xfs_db (db/malloc.c), it's not part of libxfs.
It's local to xfs_db because it is the tool that always runs out of
memory on large filesystems. It's just a wrapper that prints "out of
memory" and then exits immediately with no chance to recover.  So,
essentially, it's no different to immediately SEGV on a NULL pointer
and dying.

> Third, what problem are you solving? Granted, we should be checking every
> malloc call, and it seems that we don't.

In general, that's because we can't do anything useful to free
memory when we fail memory allocation - we generally exit
immediately on memory allocation failure, so checking for failure is
almost redundant....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/3] db: avoid to use NULL pointer

[zhanchengbin]

Changed malloc to xmalloc.

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
---
  db/block.c | 2 +-
  db/check.c | 4 ++--
  db/faddr.c | 6 +++---
  db/namei.c | 2 +-
  4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/db/block.c b/db/block.c
index f31f9544..349cf6ec 100644
--- a/db/block.c
+++ b/db/block.c
@@ -180,7 +180,7 @@ dblock_f(
  		return 0;
  	}
  	nex = nb = type == TYP_DIR2 ? mp->m_dir_geo->fsbcount : 1;
-	bmp = malloc(nb * sizeof(*bmp));
+	bmp = xmalloc(nb * sizeof(*bmp));
  	bmap(bno, nb, XFS_DATA_FORK, &nex, bmp);
  	if (nex == 0) {
  		dbprintf(_("file data block is unmapped\n"));
diff --git a/db/check.c b/db/check.c
index a9c6e632..cc3c7832 100644
--- a/db/check.c
+++ b/db/check.c
@@ -1782,7 +1782,7 @@ dir_hash_add(
  	dirhash_t		*p;

  	i = DIR_HASH_FUNC(hash, addr);
-	p = malloc(sizeof(*p));
+	p = xmalloc(sizeof(*p));
  	p->next = dirhash[i];
  	dirhash[i] = p;
  	p->hashval = hash;
@@ -3094,7 +3094,7 @@ process_leaf_node_dir_v2(
  	v2 = verbose || id->ilist;
  	v = parent = 0;
  	dbno = NULLFILEOFF;
-	freetab = malloc(FREETAB_SIZE(dirsize / mp->m_dir_geo->blksize));
+	freetab = xmalloc(FREETAB_SIZE(dirsize / mp->m_dir_geo->blksize));
  	freetab->naents = (int)(dirsize / mp->m_dir_geo->blksize);
  	freetab->nents = 0;
  	for (i = 0; i < freetab->naents; i++)
diff --git a/db/faddr.c b/db/faddr.c
index 0127c5d1..cc21faaf 100644
--- a/db/faddr.c
+++ b/db/faddr.c
@@ -137,7 +137,7 @@ fa_cfileoffd(
  		return;
  	}
  	nex = nb = next == TYP_DIR2 ? mp->m_dir_geo->fsbcount : 1;
-	bmp = malloc(nb * sizeof(*bmp));
+	bmp = xmalloc(nb * sizeof(*bmp));
  	bmap(bno, nb, XFS_DATA_FORK, &nex, bmp);
  	if (nex == 0) {
  		dbprintf(_("file block is unmapped\n"));
@@ -221,7 +221,7 @@ fa_dfiloffd(
  		return;
  	}
  	nex = nb = next == TYP_DIR2 ? mp->m_dir_geo->fsbcount : 1;
-	bmp = malloc(nb * sizeof(*bmp));
+	bmp = xmalloc(nb * sizeof(*bmp));
  	bmap(bno, nb, XFS_DATA_FORK, &nex, bmp);
  	if (nex == 0) {
  		dbprintf(_("file block is unmapped\n"));
@@ -274,7 +274,7 @@ fa_dirblock(
  		return;
  	}
  	nex = mp->m_dir_geo->fsbcount;
-	bmp = malloc(nex * sizeof(*bmp));
+	bmp = xmalloc(nex * sizeof(*bmp));
  	bmap(bno, mp->m_dir_geo->fsbcount, XFS_DATA_FORK, &nex, bmp);
  	if (nex == 0) {
  		dbprintf(_("directory block is unmapped\n"));
diff --git a/db/namei.c b/db/namei.c
index e44667a9..d1a9904b 100644
--- a/db/namei.c
+++ b/db/namei.c
@@ -266,7 +266,7 @@ dir_emit(
  		 * copy the string to a buffer so that we can add the null
  		 * terminator.
  		 */
-		display_name = malloc(namelen + 1);
+		display_name = xmalloc(namelen + 1);
  		memcpy(display_name, name, namelen);
  		display_name[namelen] = 0;
  		xname.len = namelen;
-- 
2.27.0

[PATCH 3/3] repair/slab.c: avoid to use NULL pointer

[zhanchengbin]

Changed malloc to xmalloc.

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
---
  repair/slab.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repair/slab.c b/repair/slab.c
index 165f97ef..13fc8203 100644
--- a/repair/slab.c
+++ b/repair/slab.c
@@ -237,7 +237,7 @@ qsort_slab(
  	create_work_queue(&wq, NULL, platform_nproc());
  	hdr = slab->s_first;
  	while (hdr) {
-		qs = malloc(sizeof(struct qsort_slab));
+		qs = xmalloc(sizeof(struct qsort_slab));
  		qs->slab = slab;
  		qs->hdr = hdr;
  		qs->compare_fn = compare_fn;
-- 
2.27.0