[PATCH v3 00/12] e2fsprogs: some bugfixs

[PATCH v3 00/12] e2fsprogs: some bugfixs

[wuguanghao]

From: Wu Guanghao <wuguanghao3@huawei.com>

v2->v3
[PATCH v3 04/12]ss_add_info_dir: don't zap the info->info_dirs and check whether NULL pointer
[PATCH v3 05/12]ss_create_invocation: fix memory leak and check whether NULL pointer
[PATCH v3 10/12]hashmap: change return value type of ext2fs_hashmap_add()
[PATCH v3 11/12]misc/lsattr: check whether path is NULL in lsattr_dir_proc() 
modify according to Ted's suggestion.
Other patches have been applied and not sent

v1->v2
error correction

Zhiqiang Liu (6):
  argv_parse: check return value of malloc in argv_parse()
  misc: fix potential segmentation fault problem in scandir()
  lib/ss/error.c: check return value malloc in ss_name()
  hashmap: change return value type of ext2fs_hashmap_add()
  misc/lsattr: check whether path is NULL in lsattr_dir_proc()
  ext2ed: fix potential NULL pointer dereference in dupstr()

Wu Guanghao (6):
  append_pathname: check the value returned by realloc
  profile_create_node: set magic before strdup(name) to avoid memory
    leak
  tdb_transaction_recover: fix memory leak
  zap_sector: fix memory leak
  ss_add_info_dir: don't zap the info->info_dirs and check whether NULL pointer
  ss_create_invocation: fix memory leak and check whether NULL pointer

 contrib/android/base_fs.c | 14 +++++++++----
 contrib/fsstress.c        | 10 ++++++++--
 ext2ed/main.c             |  2 ++
 lib/ext2fs/fileio.c       | 11 ++++++++--
 lib/ext2fs/hashmap.c      | 12 +++++++++--
 lib/ext2fs/hashmap.h      |  5 +++--
 lib/ext2fs/tdb.c          |  1 +
 lib/ss/error.c            |  2 ++
 lib/ss/help.c             |  5 ++++-
 lib/ss/invocation.c       | 42 ++++++++++++++++++++++++++++++---------
 lib/support/argv_parse.c  |  2 ++
 lib/support/profile.c     |  3 ++-
 misc/create_inode.c       |  3 +++
 misc/lsattr.c             |  5 +++++
 misc/mke2fs.c             |  4 +++-
 15 files changed, 97 insertions(+), 24 deletions(-)

-- 
2.27.0

[PATCH v3 04/12] ss_add_info_dir: don't zap the info->info_dirs and check whether

[wuguanghao]

From: Wu Guanghao <wuguanghao3@huawei.com>

In ss_add_info_dir(), don't zap the info->info_dirs. At the same time, it is necessary
to check whether dirs[n_dirs] is a null pointer, otherwise a segmentation
fault will occur.

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
---
 lib/ss/help.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/ss/help.c b/lib/ss/help.c
index 5204401b..b4465bfe 100644
--- a/lib/ss/help.c
+++ b/lib/ss/help.c
@@ -148,13 +148,16 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr)
     dirs = (char **)realloc((char *)dirs,
 			    (unsigned)(n_dirs + 2)*sizeof(char *));
     if (dirs == (char **)NULL) {
-	info->info_dirs = (char **)NULL;
 	*code_ptr = errno;
 	return;
     }
     info->info_dirs = dirs;
     dirs[n_dirs + 1] = (char *)NULL;
     dirs[n_dirs] = malloc((unsigned)strlen(info_dir)+1);
+    if (dirs[n_dirs] == (char *)NULL) {
+        *code_ptr = errno;
+        return;
+    }
     strcpy(dirs[n_dirs], info_dir);
     *code_ptr = 0;
 }
-- 
2.27.0

Re: [PATCH v3 04/12] ss_add_info_dir: don't zap the info->info_dirs and check whether

[Theodore Ts'o]

On Wed, Jul 28, 2021 at 09:56:45AM +0800, wuguanghao wrote:
> From: Wu Guanghao <wuguanghao3@huawei.com>
> 
> In ss_add_info_dir(), don't zap the info->info_dirs. At the same time, it is necessary
> to check whether dirs[n_dirs] is a null pointer, otherwise a segmentation
> fault will occur.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Reviewed-by: Wu Bo <wubo40@huawei.com>

Thanks, applied.

					- Ted

[PATCH v3 05/12] ss_create_invocation: fix memory leak and check whether NULL pointer

[wuguanghao]

From: Wu Guanghao <wuguanghao3@huawei.com>

In ss_create_invocation(), it is necessary to check whether
returned by malloc is a null pointer.

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 lib/ss/invocation.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/lib/ss/invocation.c b/lib/ss/invocation.c
index 457e7a2c..e458b785 100644
--- a/lib/ss/invocation.c
+++ b/lib/ss/invocation.c
@@ -26,29 +26,33 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 			 void *info_ptr, ss_request_table *request_table_ptr,
 			 int *code_ptr)
 {
-	register int sci_idx;
-	register ss_data *new_table;
-	register ss_data **table;
+	int sci_idx;
+	ss_data *new_table = NULL;
+	ss_data **table = NULL;
+	ss_data **realloc_table = NULL;
 
 	*code_ptr = 0;
 	table = _ss_table;
 	new_table = (ss_data *) malloc(sizeof(ss_data));
+	if (!new_table)
+		goto out;
 
 	if (table == (ss_data **) NULL) {
 		table = (ss_data **) malloc(2 * size);
+		if (!table)
+			goto out;
 		table[0] = table[1] = (ss_data *)NULL;
 	}
 	initialize_ss_error_table ();
 
 	for (sci_idx = 1; table[sci_idx] != (ss_data *)NULL; sci_idx++)
 		;
-	table = (ss_data **) realloc((char *)table,
+	realloc_table = (ss_data **) realloc((char *)table,
 				     ((unsigned)sci_idx+2)*size);
-	if (table == NULL) {
-		*code_ptr = ENOMEM;
-		free(new_table);
-		return 0;
-	}
+	if (realloc_table == NULL)
+		goto out;
+
+	table = realloc_table;
 	table[sci_idx+1] = (ss_data *) NULL;
 	table[sci_idx] = new_table;
 
@@ -57,9 +61,15 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 	new_table->argv = (char **)NULL;
 	new_table->current_request = (char *)NULL;
 	new_table->info_dirs = (char **)malloc(sizeof(char *));
+	if (!new_table->info_dirs)
+		goto out;
+
 	*new_table->info_dirs = (char *)NULL;
 	new_table->info_ptr = info_ptr;
 	new_table->prompt = malloc((unsigned)strlen(subsystem_name)+4);
+	if (!new_table->prompt)
+		goto out;
+
 	strcpy(new_table->prompt, subsystem_name);
 	strcat(new_table->prompt, ":  ");
 #ifdef silly
@@ -71,6 +81,9 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 	new_table->flags.abbrevs_disabled = 0;
 	new_table->rqt_tables =
 		(ss_request_table **) calloc(2, sizeof(ss_request_table *));
+	if (!new_table->rqt_tables)
+		goto out;
+
 	*(new_table->rqt_tables) = request_table_ptr;
 	*(new_table->rqt_tables+1) = (ss_request_table *) NULL;
 
@@ -85,6 +98,17 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 	ss_get_readline(sci_idx);
 #endif
 	return(sci_idx);
+
+out:
+	if (new_table) {
+		free(new_table->prompt);
+		free(new_table->info_dirs);
+	}
+	free(new_table);
+	free(table);
+	*code_ptr = ENOMEM;
+	return 0;
+
 }
 
 void
-- 
2.27.0

Re: [PATCH v3 05/12] ss_create_invocation: fix memory leak and check whether NULL pointer

[Theodore Ts'o]

On Wed, Jul 28, 2021 at 09:56:46AM +0800, wuguanghao wrote:
> From: Wu Guanghao <wuguanghao3@huawei.com>
> 
> In ss_create_invocation(), it is necessary to check whether
> returned by malloc is a null pointer.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>

Thanks, applied.

					- Ted

[PATCH v3 10/12] hashmap: change return value type of ext2fs_hashmap_add()

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

In ext2fs_hashmap_add(), new entry is allocated by calling
malloc(). If malloc() return NULL, it will cause a
segmentation fault problem.

Here, we change return value type of ext2fs_hashmap_add()
from void to int. If allocating new entry fails, we will
return 1, and the callers should also verify the return
value of ext2fs_hashmap_add().

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 contrib/android/base_fs.c | 14 ++++++++++----
 lib/ext2fs/fileio.c       | 10 ++++++++--
 lib/ext2fs/hashmap.c      | 12 ++++++++++--
 lib/ext2fs/hashmap.h      |  5 +++--
 4 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/contrib/android/base_fs.c b/contrib/android/base_fs.c
index 652317e2..4ed44169 100644
--- a/contrib/android/base_fs.c
+++ b/contrib/android/base_fs.c
@@ -100,7 +100,7 @@ static void free_base_fs_entry(void *e)
 
 struct ext2fs_hashmap *basefs_parse(const char *file, const char *mountpoint)
 {
-	int err;
+	errcode_t err;
 	struct ext2fs_hashmap *entries = NULL;
 	struct basefs_entry *entry;
 	FILE *f = basefs_open(file);
@@ -110,10 +110,16 @@ struct ext2fs_hashmap *basefs_parse(const char *file, const char *mountpoint)
 	if (!entries)
 		goto end;
 
-	while ((entry = basefs_readline(f, mountpoint, &err)))
-		ext2fs_hashmap_add(entries, entry, entry->path,
+	while ((entry = basefs_readline(f, mountpoint, &err))) {
+		err = ext2fs_hashmap_add(entries, entry, entry->path,
 				   strlen(entry->path));
-
+		if (err) {
+			free_base_fs_entry(entry);
+			fclose(f);
+			ext2fs_hashmap_free(entries);
+			return NULL;
+		}
+	}
 	if (err) {
 		fclose(f);
 		ext2fs_hashmap_free(entries);
diff --git a/lib/ext2fs/fileio.c b/lib/ext2fs/fileio.c
index a0b5d971..2b0fb91e 100644
--- a/lib/ext2fs/fileio.c
+++ b/lib/ext2fs/fileio.c
@@ -475,8 +475,14 @@ errcode_t ext2fs_file_write(ext2_file_t file, const void *buf,
 
 			if (new_block) {
 				new_block->physblock = file->physblock;
-				ext2fs_hashmap_add(fs->block_sha_map, new_block,
-					new_block->sha, sizeof(new_block->sha));
+				errcode_t ret = ext2fs_hashmap_add(fs->block_sha_map,
+						new_block, new_block->sha,
+						sizeof(new_block->sha));
+				if (ret) {
+					retval = EXT2_ET_NO_MEMORY;
+					free(new_block);
+					goto fail;
+				}
 			}
 
 			if (bmap_flags & BMAP_SET) {
diff --git a/lib/ext2fs/hashmap.c b/lib/ext2fs/hashmap.c
index ffe61ce9..5239c921 100644
--- a/lib/ext2fs/hashmap.c
+++ b/lib/ext2fs/hashmap.c
@@ -36,6 +36,9 @@ struct ext2fs_hashmap *ext2fs_hashmap_create(
 {
 	struct ext2fs_hashmap *h = calloc(sizeof(struct ext2fs_hashmap) +
 				sizeof(struct ext2fs_hashmap_entry) * size, 1);
+	if (!h)
+		return NULL;
+
 	h->size = size;
 	h->free = free_fct;
 	h->hash = hash_fct;
@@ -43,12 +46,15 @@ struct ext2fs_hashmap *ext2fs_hashmap_create(
 	return h;
 }
 
-void ext2fs_hashmap_add(struct ext2fs_hashmap *h, void *data, const void *key,
-			size_t key_len)
+errcode_t ext2fs_hashmap_add(struct ext2fs_hashmap *h,
+		void *data, const void *key, size_t key_len)
 {
 	uint32_t hash = h->hash(key, key_len) % h->size;
 	struct ext2fs_hashmap_entry *e = malloc(sizeof(*e));
 
+	if (!e)
+		return -1;
+
 	e->data = data;
 	e->key = key;
 	e->key_len = key_len;
@@ -62,6 +68,8 @@ void ext2fs_hashmap_add(struct ext2fs_hashmap *h, void *data, const void *key,
 	h->first = e;
 	if (!h->last)
 		h->last = e;
+
+	return 0;
 }
 
 void *ext2fs_hashmap_lookup(struct ext2fs_hashmap *h, const void *key,
diff --git a/lib/ext2fs/hashmap.h b/lib/ext2fs/hashmap.h
index dcfa7455..43669a42 100644
--- a/lib/ext2fs/hashmap.h
+++ b/lib/ext2fs/hashmap.h
@@ -3,6 +3,7 @@
 
 # include <stdlib.h>
 # include <stdint.h>
+# include "et/com_err.h"
 
 #ifndef __GNUC_PREREQ
 #if defined(__GNUC__) && defined(__GNUC_MINOR__)
@@ -27,8 +28,8 @@ struct ext2fs_hashmap_entry {
 struct ext2fs_hashmap *ext2fs_hashmap_create(
 				uint32_t(*hash_fct)(const void*, size_t),
 				void(*free_fct)(void*), size_t size);
-void ext2fs_hashmap_add(struct ext2fs_hashmap *h, void *data, const void *key,
-			size_t key_len);
+errcode_t ext2fs_hashmap_add(struct ext2fs_hashmap *h,
+		       void *data, const void *key,size_t key_len);
 void *ext2fs_hashmap_lookup(struct ext2fs_hashmap *h, const void *key,
 			    size_t key_len);
 void *ext2fs_hashmap_iter_in_order(struct ext2fs_hashmap *h,
-- 
2.27.0

Re: [PATCH v3 10/12] hashmap: change return value type of ext2fs_hashmap_add()

[Theodore Ts'o]

On Wed, Jul 28, 2021 at 09:56:47AM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In ext2fs_hashmap_add(), new entry is allocated by calling
> malloc(). If malloc() return NULL, it will cause a
> segmentation fault problem.
> 
> Here, we change return value type of ext2fs_hashmap_add()
> from void to int. If allocating new entry fails, we will
> return 1, and the callers should also verify the return
> value of ext2fs_hashmap_add().
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>

Thanks, applied.

Note: I changed ext2fs_hashmap_add() to return an int instead of an
errocode_t.  The commit description said it was going to be an int,
and the code returns -1 (so I fixed the commit description to reflect
-1).  Note that errcode_t is not appropriate for non-errno / com_err
error codes.  So making the function prototype of hashmap_add() to
return an int is the correct thing to do.

Cheers,

					- Ted

[PATCH v3 11/12] misc/lsattr: check whether path is NULL in lsattr_dir_proc()

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

In lsattr_dir_proc(), if malloc() return NULL, it will cause
a segmentation fault problem.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 misc/lsattr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/misc/lsattr.c b/misc/lsattr.c
index 0d954376..7958f0ad 100644
--- a/misc/lsattr.c
+++ b/misc/lsattr.c
@@ -144,6 +144,11 @@ static int lsattr_dir_proc (const char * dir_name, struct dirent * de,
 	int dir_len = strlen(dir_name);
 
 	path = malloc(dir_len + strlen (de->d_name) + 2);
+	if (!path) {
+		fputs(_("Couldn't allocate path variable in lsattr_dir_proc"),
+			stderr);
+		return -1;
+	}
 
 	if (dir_len && dir_name[dir_len-1] == '/')
 		sprintf (path, "%s%s", dir_name, de->d_name);
-- 
2.27.0

Re: [PATCH v3 11/12] misc/lsattr: check whether path is NULL in lsattr_dir_proc()

[Theodore Ts'o]

On Wed, Jul 28, 2021 at 09:56:48AM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In lsattr_dir_proc(), if malloc() return NULL, it will cause
> a segmentation fault problem.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>

Thanks, applied.

Note that fputs() does not print a trailing newline (unlike puts()).
So I fixed up the error message to include a newline character at the
end.

				- Ted