[PATCH] erofs-utils: fix unchecked strdup() and harden erofs_fspath()

Saksham <[email protected]>
Newsgroups org.ozlabs.lists.linux-erofs
Message-ID <[email protected]>
In several places across the erofs-utils codebase, the return value of
strdup() was not checked for failure. This can lead to NULL pointer
dereferences or unexpected behavior when these strings are processed
downstream, especially in utility functions like erofs_fspath().

Specifically, erofs_make_empty_root_inode() in lib/inode.c assigned
the result of strdup("/") to root->i_srcpath without validation. If
this allocation failed, subsequent calls to erofs_fspath(inode->i_srcpath)
would attempt to dereference a NULL pointer (or an offset from NULL),
potentially causing a crash or memory safety issues.

This patch addresses these issues by:
1. Adding proper NULL checks for strdup() in erofs_make_empty_root_inode()
   (lib/inode.c) and erofs_rebuild_load_tree() (lib/rebuild.c). If
   allocation fails, the functions now correctly return -ENOMEM.
2. Hardening erofs_fspath() in lib/config.c to gracefully handle NULL
   inputs. It now returns an empty string instead of attempting to
   dereference the pointer, providing a robust fallback for memory
   exhaustion scenarios.
3. Fixing unchecked strdup() and strndup() calls in the S3 remote
   backend (lib/remotes/s3.c) and mkfs (mkfs/main.c).

These changes improve the overall robustness and reliability of the
erofs-utils suite, particularly when operating under memory pressure.

Detailed changes:
- lib/inode.c: Check strdup("/") in erofs_make_empty_root_inode;
  call erofs_iput() and return ERR_PTR(-ENOMEM) on failure.
- lib/rebuild.c: Check strdup("/") in erofs_rebuild_load_tree;
  return -ENOMEM on failure.
- lib/config.c: Add NULL check to erofs_fspath().
- mkfs/main.c: Check strdup(extraopts) in mkfs_parse_one_compress_alg.
- lib/remotes/s3.c: Check strdup() in s3erofs_parse_list_objects_one()
  and s3erofs_create_object_iterator(). Properly clean up and return
  errors.

Signed-off-by: Saksham <[email protected]>
---
 lib/config.c     |  7 +++++--
 lib/inode.c      |  5 +++++
 lib/rebuild.c    |  2 ++
 lib/remotes/s3.c | 24 ++++++++++++++++++++----
 mkfs/main.c      |  5 ++++-
 5 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/lib/config.c b/lib/config.c
index ab7eb01..b26dd57 100644
--- a/lib/config.c
+++ b/lib/config.c
@@ -69,13 +69,16 @@ void erofs_set_fs_root(const char *rootdir)
 
 const char *erofs_fspath(const char *fullpath)
 {
-	const char *s = fullpath + fullpath_prefix;
+	const char *s;
 
+	if (!fullpath)
+		return "";
+
+	s = fullpath + fullpath_prefix;
 	while (*s == '/')
 		s++;
 	return s;
 }
-
 #ifdef HAVE_LIBSELINUX
 int erofs_selabel_open(const char *file_contexts)
 {
diff --git a/lib/inode.c b/lib/inode.c
index 2cfc6c5..74420b2 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -2449,7 +2449,12 @@ struct erofs_inode *erofs_make_empty_root_inode(struct erofs_importer *im,
 	root = erofs_new_inode(sbi);
 	if (IS_ERR(root))
 		return root;
+
 	root->i_srcpath = strdup("/");
+	if (!root->i_srcpath) {
+		erofs_iput(root);
+		return ERR_PTR(-ENOMEM);
+	}
 	root->i_mode = S_IFDIR | 0777;
 	root->i_uid = (!params || params->fixed_uid == -1) ? getuid() :
 							     params->fixed_uid;
diff --git a/lib/rebuild.c b/lib/rebuild.c
index f89a17c..f40bf23 100644
--- a/lib/rebuild.c
+++ b/lib/rebuild.c
@@ -446,6 +446,8 @@ int erofs_rebuild_load_tree(struct erofs_inode *root, struct erofs_sb_info *sbi,
 		return ret;
 	}
 	inode.i_srcpath = strdup("/");
+	if (!inode.i_srcpath)
+		return -ENOMEM;
 
 	ctx = (struct erofs_rebuild_dir_context) {
 		.ctx.dir = &inode,
diff --git a/lib/remotes/s3.c b/lib/remotes/s3.c
index 768232a..0dbe5aa 100644
--- a/lib/remotes/s3.c
+++ b/lib/remotes/s3.c
@@ -714,9 +714,13 @@ static int s3erofs_parse_list_objects_one(xmlNodePtr node,
 				tm.tm_isdst = -1;
 				info->mtime = mktime(&tm);
 			}
-			if (xmlStrEqual(child->name, (const xmlChar *)"Key"))
+			if (xmlStrEqual(child->name, (const xmlChar *)"Key")) {
 				info->key = strdup((char *)str);
-			else if (xmlStrEqual(child->name, (const xmlChar *)"Size"))
+				if (!info->key) {
+					xmlFree(str);
+					return -ENOMEM;
+				}
+			} else if (xmlStrEqual(child->name, (const xmlChar *)"Size"))
 				info->size = atoll((char *)str);
 			xmlFree(str);
 		}
@@ -898,6 +902,7 @@ s3erofs_create_object_iterator(struct erofs_s3 *s3, const char *path,
 {
 	struct s3erofs_object_iterator *iter;
 	const char *prefix;
+	int ret;
 
 	iter = calloc(1, sizeof(struct s3erofs_object_iterator));
 	if (!iter)
@@ -911,14 +916,25 @@ s3erofs_create_object_iterator(struct erofs_s3 *s3, const char *path,
 		iter->bucket = NULL;
 		iter->prefix = strdup(path + 1);
 	} else {
-		if (++prefix - path > S3EROFS_PATH_MAX)
-			return ERR_PTR(-EINVAL);
+		if (++prefix - path > S3EROFS_PATH_MAX) {
+			ret = -EINVAL;
+			goto err;
+		}
 		iter->bucket = strndup(path, prefix - path);
 		iter->prefix = strdup(prefix);
 	}
+
+	if ((!iter->bucket && prefix != path) ||
+	    (!iter->prefix && prefix)) {
+		ret = -ENOMEM;
+		goto err;
+	}
 	iter->delimiter = delimiter;
 	iter->is_truncated = true;
 	return iter;
+err:
+	s3erofs_destroy_object_iterator(iter);
+	return ERR_PTR(ret);
 }
 
 static void s3erofs_destroy_object_iterator(struct s3erofs_object_iterator *it)
diff --git a/mkfs/main.c b/mkfs/main.c
index 58c18f9..d759144 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -987,8 +987,11 @@ static int mkfs_parse_one_compress_alg(char *alg)
 			}
 		}
 	}
-	if (i)
+	if (i) {
 		zset->extraopts = strdup(extraopts);
+		if (!zset->extraopts)
+			return -ENOMEM;
+	}
 	return mkfscfg.total_zcfgs++;
 }
 
-- 
2.53.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.