[PATCH 00/12] e2fsprogs: some bugfixs and some code cleanups

[PATCH 00/12] e2fsprogs: some bugfixs and some code cleanups

[Wu Guanghao]

When we learn the e2fsprogs source code, we
find some bugs and try to fix them. At the same time,
static scanning tools were used to analyze the code,
and several possible problems were found.

Zhiqiang Liu (6):
  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()
  argv_parse: check return value of malloc in argv_pars

Wu Guanghao (6):
  profile_create_node: set magic before strdup(name) to fix memory leak
  tdb_transaction_recover: fix memory leak
  zap_sector: fix memory leak
  ss_add_info_dir: fix memory leak and check whether NULL pointer
  ss_create_invocation: fix memory leak and check whether NULL pointer
  append_pathname: append_pathname: check the value returned by realloc
    to avoid segfault

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

-- 
2.19.1

[PATCH 01/12] profile_create_node: set magic before strdup(name) to avoid memory leak

[Wu Guanghao]

If new->magic != PROF_MAGIC_NODE, profile_free_node() don't free node.
This will cause the node to be unable to be released correctly and
a memory leak 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/support/profile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/support/profile.c b/lib/support/profile.c
index 585ed595..2eb3a9d1 100644
--- a/lib/support/profile.c
+++ b/lib/support/profile.c
@@ -1093,6 +1093,8 @@ errcode_t profile_create_node(const char *name, const char *value,
 	if (!new)
 		return ENOMEM;
 	memset(new, 0, sizeof(struct profile_node));
+	new->magic = PROF_MAGIC_NODE;
+
 	new->name = strdup(name);
 	if (new->name == 0) {
 	    profile_free_node(new);
@@ -1105,7 +1107,6 @@ errcode_t profile_create_node(const char *name, const char *value,
 		    return ENOMEM;
 		}
 	}
-	new->magic = PROF_MAGIC_NODE;

 	*ret_node = new;
 	return 0;
-- 

[PATCH 02/12] tdb_transaction_recover: fix memory leak

[Wu Guanghao]

In tdb_transaction_recover(), need free data before return,
otherwise it will cause memory leak.

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
---
 lib/ext2fs/tdb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/ext2fs/tdb.c b/lib/ext2fs/tdb.c
index 5091b128..ddfcf130 100644
--- a/lib/ext2fs/tdb.c
+++ b/lib/ext2fs/tdb.c
@@ -2186,6 +2186,7 @@ int tdb_transaction_recover(struct tdb_context *tdb)
 				   rec.data_len, 0) == -1) {
 		TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_recover: failed to read recovery data\n"));
 		tdb->ecode = TDB_ERR_IO;
+		free(data);
 		return -1;
 	}

-- 

[PATCH 03/12] zap_sector: fix memory leak

[Wu Guanghao]

In zap_sector(), need free buf before return,
otherwise it will cause memory leak.

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
---
 misc/mke2fs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index afbcf486..94f81da9 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -586,6 +586,7 @@ static void zap_sector(ext2_filsys fs, int sect, int nsect)
 			magic = (unsigned int *) (buf + BSD_LABEL_OFFSET);
 			if ((*magic == BSD_DISKMAGIC) ||
 				(*magic == BSD_MAGICDISK))
+				free(buf);
 				return;
 		}
 	}
-- 

[PATCH 04/12] ss_add_info_dir: fix memory leak and check whether,NULL pointer

[Wu Guanghao]

In ss_add_info_dir(), need free info->info_dirs before return,
otherwise it will cause memory leak. 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, 5 insertions(+)

diff --git a/lib/ss/help.c b/lib/ss/help.c
index 5204401b..429f410e 100644
--- a/lib/ss/help.c
+++ b/lib/ss/help.c
@@ -148,6 +148,7 @@ 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) {
+	free(info->info_dirs);
 	info->info_dirs = (char **)NULL;
 	*code_ptr = errno;
 	return;
@@ -155,6 +156,10 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr)
     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;
 }
-- 

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

[Wu Guanghao]

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 | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/lib/ss/invocation.c b/lib/ss/invocation.c
index 457e7a2c..cfc180a5 100644
--- a/lib/ss/invocation.c
+++ b/lib/ss/invocation.c
@@ -29,26 +29,31 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 	register int sci_idx;
 	register ss_data *new_table;
 	register ss_data **table;
+	register ss_data **realloc_table;

 	*code_ptr = 0;
 	table = _ss_table;
 	new_table = (ss_data *) malloc(sizeof(ss_data));
+	if (new_table == NULL)
+		goto out;

 	if (table == (ss_data **) NULL) {
 		table = (ss_data **) malloc(2 * size);
+		if (table == NULL)
+			goto free_new_table;
 		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 free_table;
+
+	table = realloc_table;
+
 	table[sci_idx+1] = (ss_data *) NULL;
 	table[sci_idx] = new_table;

@@ -57,9 +62,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 == NULL)
+		goto free_table;
+
 	*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 == NULL)
+		goto free_info_dirs;
+
 	strcpy(new_table->prompt, subsystem_name);
 	strcat(new_table->prompt, ":  ");
 #ifdef silly
@@ -71,6 +82,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 == NULL)
+		goto free_prompt;
+
 	*(new_table->rqt_tables) = request_table_ptr;
 	*(new_table->rqt_tables+1) = (ss_request_table *) NULL;

@@ -85,6 +99,18 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
 	ss_get_readline(sci_idx);
 #endif
 	return(sci_idx);
+
+free_prompt:
+	free(new_table->prompt);
+free_info_dirs:
+	free(new_table->info_dirs);
+free_table:
+	free(table);
+free_new_table:
+	free(new_table);
+out:
+	*code_ptr = ENOMEM;
+	return 0;
 }

 void
-- 

[PATCH 06/12] append_pathname: check the value returned by realloc to avoid segfault

[Wu Guanghao]

In append_pathname(), we need to add a new path to save the value returned by realloc,
otherwise the name->path may be NULL, causing segfault

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 contrib/fsstress.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/contrib/fsstress.c b/contrib/fsstress.c
index 2a983482..530bd920 100644
--- a/contrib/fsstress.c
+++ b/contrib/fsstress.c
@@ -599,7 +599,7 @@ void add_to_flist(int ft, int id, int parent)
 void append_pathname(pathname_t * name, char *str)
 {
 	int len;
-
+	char *path;
 	len = strlen(str);
 #ifdef DEBUG
 	if (len && *str == '/' && name->len == 0) {
@@ -609,7 +609,13 @@ void append_pathname(pathname_t * name, char *str)

 	}
 #endif
-	name->path = realloc(name->path, name->len + 1 + len);
+	path = realloc(name->path, name->len + 1 + len);
+	if (path == NULL) {
+		fprintf(stderr, "fsstress: append_pathname realloc failed\n");
+		chadir(homedir);
+		abort();
+	}
+	name->path = path;
 	strcpy(&name->path[name->len], str);
 	name->len += len;
 }
-- 

[PATCH 07/12] argv_parse: check return value of malloc in argv_parse()

[Wu Guanghao]

In argv_parse(), return value of malloc should be checked
whether it is NULL, otherwise, it may cause a segfault error.

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

diff --git a/lib/support/argv_parse.c b/lib/support/argv_parse.c
index d22f6344..1ef9c014 100644
--- a/lib/support/argv_parse.c
+++ b/lib/support/argv_parse.c
@@ -116,6 +116,8 @@ int argv_parse(char *in_buf, int *ret_argc, char ***ret_argv)
 	if (argv == 0) {
 		argv = malloc(sizeof(char *));
 		free(buf);
+		if (!arcv)
+			return -1;
 	}
 	argv[argc] = 0;
 	if (ret_argc)
-- 

[PATCH 08/12] misc: fix potential segmentation fault problem in, scandir()

[Wu Guanghao]

In scandir(), temp_list[num_dent] is allocated by calling
malloc(), we should check whether malloc() returns NULL before
accessing temp_list[num_dent].

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

diff --git a/misc/create_inode.c b/misc/create_inode.c
index d62e1cb4..869b0614 100644
--- a/misc/create_inode.c
+++ b/misc/create_inode.c
@@ -771,6 +771,9 @@ static int scandir(const char *dir_name, struct dirent ***name_list,
 		}
 		// add the copy of dirent to the list
 		temp_list[num_dent] = (struct dirent*)malloc((dent->d_reclen + 3) & ~3);
+		if (!temp_list[num_dent]) {
+			goto out;
+		}
 		memcpy(temp_list[num_dent], dent, dent->d_reclen);
 		num_dent++;
 	}
-- 

[PATCH 09/12] lib/ss/error.c: check return value malloc in ss_name()

[Wu Guanghao]

In ss_name(), we should check return value of malloc(),
otherwise, it may cause a segmentation fault problem.

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

diff --git a/lib/ss/error.c b/lib/ss/error.c
index 8d345a9f..656b71be 100644
--- a/lib/ss/error.c
+++ b/lib/ss/error.c
@@ -42,6 +42,8 @@ char *ss_name(int sci_idx)
 			 (strlen(infop->subsystem_name)+
 			  strlen(infop->current_request)+
 			  4));
+	if (ret_val == (char *)NULL)
+		return ((char *)NULL);
 	cp = ret_val;
 	cp1 = infop->subsystem_name;
 	while (*cp1)
-- 

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

[Wu Guanghao]

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 | 12 +++++++++---
 lib/ext2fs/fileio.c       | 11 +++++++++--
 lib/ext2fs/hashmap.c      | 12 ++++++++++--
 lib/ext2fs/hashmap.h      |  4 ++--
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/contrib/android/base_fs.c b/contrib/android/base_fs.c
index 652317e2..d3e00d18 100644
--- a/contrib/android/base_fs.c
+++ b/contrib/android/base_fs.c
@@ -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..b86951d9 100644
--- a/lib/ext2fs/fileio.c
+++ b/lib/ext2fs/fileio.c
@@ -475,8 +475,15 @@ 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));
+				int 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);
+					new_block = NULL;
+					goto fail;
+				}
 			}

 			if (bmap_flags & BMAP_SET) {
diff --git a/lib/ext2fs/hashmap.c b/lib/ext2fs/hashmap.c
index ffe61ce9..7278edaf 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)
+int 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..0c09d2bd 100644
--- a/lib/ext2fs/hashmap.h
+++ b/lib/ext2fs/hashmap.h
@@ -27,8 +27,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);
+int 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,
-- 

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

[Wu Guanghao]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/misc/lsattr.c b/misc/lsattr.c
index 0d954376..f3212069 100644
--- a/misc/lsattr.c
+++ b/misc/lsattr.c
@@ -144,6 +144,12 @@ 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) {
+		fprintf(stderr, "%s",
+			_("Couldn't allocate path variable "
+			  "in lsattr_dir_proc"));
+		return -1;
+	}

 	if (dir_len && dir_name[dir_len-1] == '/')
 		sprintf (path, "%s%s", dir_name, de->d_name);
-- 

[PATCH 12/12] ext2ed: fix potential NULL pointer dereference in, dupstr()

[Wu Guanghao]

In dupstr(), we should check return value of malloc().

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
---
 ext2ed/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext2ed/main.c b/ext2ed/main.c
index f7e7d7df..9d33a8e1 100644
--- a/ext2ed/main.c
+++ b/ext2ed/main.c
@@ -524,6 +524,8 @@ char *dupstr (char *src)
 	char *ptr;

 	ptr=(char *) malloc (strlen (src)+1);
+	if (!ptr)
+		return NULL;
 	strcpy (ptr,src);
 	return (ptr);
 }
-- 

Re: [PATCH 03/12] zap_sector: fix memory leak

[Благодаренко Артём]

Hello Wu,

Thank you for the fixes.

It looks like free and return operators should be placed in {} block.

{
	free(buf);
 	return;
}

Now function returns any time block is read successfully.

Also, this patch can not be applied cleanly to the master HEAD because of wrong offsets. Please rebase.

Best regards,
Artem Blagodarenko.

> On 24 May 2021, at 14:20, Wu Guanghao <wuguanghao3@huawei.com> wrote:
> 
> In zap_sector(), need free buf before return,
> otherwise it will cause memory leak.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Reviewed-by: Wu Bo <wubo40@huawei.com>
> ---
> misc/mke2fs.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index afbcf486..94f81da9 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -586,6 +586,7 @@ static void zap_sector(ext2_filsys fs, int sect, int nsect)
> 			magic = (unsigned int *) (buf + BSD_LABEL_OFFSET);
> 			if ((*magic == BSD_DISKMAGIC) ||
> 				(*magic == BSD_MAGICDISK))
> +				free(buf);
> 				return;
> 		}
> 	}
> -- 

Re: [PATCH 03/12] zap_sector: fix memory leak

[Wu Guanghao]

Thank you for your comments, I will modify it in the v2 version.

Best regards
Wu Guanghao

在 2021/5/24 22:40, Благодаренко Артём 写道:
> Hello Wu,
> 
> Thank you for the fixes.
> 
> It looks like free and return operators should be placed in {} block.
> 
> {
> 	free(buf);
>  	return;
> }
> 
> Now function returns any time block is read successfully.
> 
> Also, this patch can not be applied cleanly to the master HEAD because of wrong offsets. Please rebase.
> 
> Best regards,
> Artem Blagodarenko.
> 
>> On 24 May 2021, at 14:20, Wu Guanghao <wuguanghao3@huawei.com> wrote:
>>
>> In zap_sector(), need free buf before return,
>> otherwise it will cause memory leak.
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Reviewed-by: Wu Bo <wubo40@huawei.com>
>> ---
>> misc/mke2fs.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
>> index afbcf486..94f81da9 100644
>> --- a/misc/mke2fs.c
>> +++ b/misc/mke2fs.c
>> @@ -586,6 +586,7 @@ static void zap_sector(ext2_filsys fs, int sect, int nsect)
>> 			magic = (unsigned int *) (buf + BSD_LABEL_OFFSET);
>> 			if ((*magic == BSD_DISKMAGIC) ||
>> 				(*magic == BSD_MAGICDISK))
>> +				free(buf);
>> 				return;
>> 		}
>> 	}
>> -- 
> 
> .
> 

Re: [PATCH 07/12] argv_parse: check return value of malloc in argv_parse()

[Wu Guanghao]

There is a problem with this patch. "argv" is spelled as "arcv".
This problem will be resolved in the v2 version.

在 2021/5/24 19:23, Wu Guanghao 写道:
> In argv_parse(), return value of malloc should be checked
> whether it is NULL, otherwise, it may cause a segfault error.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  lib/support/argv_parse.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/support/argv_parse.c b/lib/support/argv_parse.c
> index d22f6344..1ef9c014 100644
> --- a/lib/support/argv_parse.c
> +++ b/lib/support/argv_parse.c
> @@ -116,6 +116,8 @@ int argv_parse(char *in_buf, int *ret_argc, char ***ret_argv)
>  	if (argv == 0) {
>  		argv = malloc(sizeof(char *));
>  		free(buf);
> +		if (!arcv)
> +			return -1;
>  	}
>  	argv[argc] = 0;
>  	if (ret_argc)
> 

Re: [PATCH 06/12] append_pathname: check the value returned by realloc to avoid segfault

[Благодаренко Артём]

Hello Wu,

Thanks for the patch.

> On 24 May 2021, at 14:23, Wu Guanghao <wuguanghao3@huawei.com> wrote:
> 
> In append_pathname(), we need to add a new path to save the value returned by realloc,
> otherwise the name->path may be NULL, causing segfault
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> contrib/fsstress.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/fsstress.c b/contrib/fsstress.c
> index 2a983482..530bd920 100644
> --- a/contrib/fsstress.c
> +++ b/contrib/fsstress.c
> @@ -599,7 +599,7 @@ void add_to_flist(int ft, int id, int parent)
> void append_pathname(pathname_t * name, char *str)
> {
> 	int len;
> -
> +	char *path;:
> 	len = strlen(str);
> #ifdef DEBUG
> 	if (len && *str == '/' && name->len == 0) {
> @@ -609,7 +609,13 @@ void append_pathname(pathname_t * name, char *str)
> 
> 	}
> #endif
> -	name->path = realloc(name->path, name->len + 1 + len);
> +	path = realloc(name->path, name->len + 1 + len);
> +	if (path == NULL) {
> +		fprintf(stderr, "fsstress: append_pathname realloc failed\n");
> +		chadir(homedir);

Did you mean chdir() here?

> +		abort();
> +	}
> +	name->path = path;
> 	strcpy(&name->path[name->len], str);
> 	name->len += len;
> }
> -- 


Thanks,
Best regards,
Artem Blagodarenko.

Re: [PATCH 06/12] append_pathname: check the value returned by realloc to avoid segfault

[Wu Guanghao]

Hello Artem Blagodarenko,

Thank you for your review, it should be chdir instead of chadir,
I will modify it in the v2 version. Next time I will take care not to make similar mistakes.
Thanks a lot.

Thanks,
Best regards,
Wu Guanghao

在 2021/5/25 17:37, Благодаренко Артём 写道:
> Hello Wu,
> 
> Thanks for the patch.
> 
>> On 24 May 2021, at 14:23, Wu Guanghao <wuguanghao3@huawei.com> wrote:
>>
>> In append_pathname(), we need to add a new path to save the value returned by realloc,
>> otherwise the name->path may be NULL, causing segfault
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> contrib/fsstress.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/fsstress.c b/contrib/fsstress.c
>> index 2a983482..530bd920 100644
>> --- a/contrib/fsstress.c
>> +++ b/contrib/fsstress.c
>> @@ -599,7 +599,7 @@ void add_to_flist(int ft, int id, int parent)
>> void append_pathname(pathname_t * name, char *str)
>> {
>> 	int len;
>> -
>> +	char *path;:
>> 	len = strlen(str);
>> #ifdef DEBUG
>> 	if (len && *str == '/' && name->len == 0) {
>> @@ -609,7 +609,13 @@ void append_pathname(pathname_t * name, char *str)
>>
>> 	}
>> #endif
>> -	name->path = realloc(name->path, name->len + 1 + len);
>> +	path = realloc(name->path, name->len + 1 + len);
>> +	if (path == NULL) {
>> +		fprintf(stderr, "fsstress: append_pathname realloc failed\n");
>> +		chadir(homedir);
> 
> Did you mean chdir() here?
> 
>> +		abort();
>> +	}
>> +	name->path = path;
>> 	strcpy(&name->path[name->len], str);
>> 	name->len += len;
>> }
>> -- 
> 
> 
> Thanks,
> Best regards,
> Artem Blagodarenko.
> 
> .
> 

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

[Благодаренко Артём]

> On 24 May 2021, at 14:25, Wu Guanghao <wuguanghao3@huawei.com> wrote:
> 
> 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 | 12 +++++++++---
> lib/ext2fs/fileio.c       | 11 +++++++++--
> lib/ext2fs/hashmap.c      | 12 ++++++++++--
> lib/ext2fs/hashmap.h      |  4 ++--
> 4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/contrib/android/base_fs.c b/contrib/android/base_fs.c
> index 652317e2..d3e00d18 100644
> --- a/contrib/android/base_fs.c
> +++ b/contrib/android/base_fs.c
> @@ -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..b86951d9 100644
> --- a/lib/ext2fs/fileio.c
> +++ b/lib/ext2fs/fileio.c
> @@ -475,8 +475,15 @@ 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));
> +				int 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);
> +					new_block = NULL;

There is no need to set new_block to NULL here.. new_block is a local variable and the function returns after "fail" label.
Same for blocks above which also jump to the “fail” label.
> +					goto fail;
> +				}
> 			}
> 
> 			if (bmap_flags & BMAP_SET) {
> diff --git a/lib/ext2fs/hashmap.c b/lib/ext2fs/hashmap.c
> index ffe61ce9..7278edaf 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)
> +int 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..0c09d2bd 100644
> --- a/lib/ext2fs/hashmap.h
> +++ b/lib/ext2fs/hashmap.h
> @@ -27,8 +27,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);
> +int 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,
> -- 

Best regards,
Artem Blagodarenko

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

[Zhiqiang Liu]

On 2021/5/25 20:45, Благодаренко Артём wrote:
>
>> On 24 May 2021, at 14:25, Wu Guanghao <wuguanghao3@huawei.com> wrote:
>>
>> 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 | 12 +++++++++---
>> lib/ext2fs/fileio.c       | 11 +++++++++--
>> lib/ext2fs/hashmap.c      | 12 ++++++++++--
>> lib/ext2fs/hashmap.h      |  4 ++--
>> 4 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/contrib/android/base_fs.c b/contrib/android/base_fs.c
>> index 652317e2..d3e00d18 100644
>> --- a/contrib/android/base_fs.c
>> +++ b/contrib/android/base_fs.c
>> @@ -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..b86951d9 100644
>> --- a/lib/ext2fs/fileio.c
>> +++ b/lib/ext2fs/fileio.c
>> @@ -475,8 +475,15 @@ 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));
>> +				int 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);
>> +					new_block = NULL;
> There is no need to set new_block to NULL here.. new_block is a local variable and the function returns after "fail" label.
> Same for blocks above which also jump to the “fail” label.

Thanks for your suggestion.

we will send the v2 patches.

>> +					goto fail;
>> +				}
>> 			}
>>
>> 			if (bmap_flags & BMAP_SET) {
>> diff --git a/lib/ext2fs/hashmap.c b/lib/ext2fs/hashmap.c
>> index ffe61ce9..7278edaf 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)
>> +int 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..0c09d2bd 100644
>> --- a/lib/ext2fs/hashmap.h
>> +++ b/lib/ext2fs/hashmap.h
>> @@ -27,8 +27,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);
>> +int 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,
>> -- 
> Best regards,
> Artem Blagodarenko
>
>
> .