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

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

[Wu Guanghao]

V1 -> V2:

[PATCH V2 03/12] zap_sector: fix memory leak
	free and return operators placed in {} block

[PATCH V2 04/12] ss_add_info_dir: fix memory leak and check whether,NULL pointer
	modified "=" to "=="

[PATCH V2 06/12] append_pathname: check the value returned by realloc to avoid segfault
[PATCH V2 07/12] argv_parse: check return value of malloc in argv_parse()
	Fix typos

[PATCH V2 10/12] hashmap: change return value type of, ext2fs_hashmap_add()
	remove "new_block = NULL;"

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 V2 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index afbcf486..2f229534 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -585,8 +585,10 @@ static void zap_sector(ext2_filsys fs, int sect, int nsect)
 		else {
 			magic = (unsigned int *) (buf + BSD_LABEL_OFFSET);
 			if ((*magic == BSD_DISKMAGIC) ||
-			    (*magic == BSD_MAGICDISK))
+			    (*magic == BSD_MAGICDISK)) {
+				free(buf);
 				return;
+			}
 		}
 	}

-- 

[PATCH V2 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 V2 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");
+		chdir(homedir);
+		abort();
+	}
+	name->path = path;
 	strcpy(&name->path[name->len], str);
 	name->len += len;
 }
-- 

[PATCH V2 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 (!argv)
+			return -1;
 	}
 	argv[argc] = 0;
 	if (ret_argc)
-- 

[PATCH V2 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       | 10 ++++++++--
 lib/ext2fs/hashmap.c      | 12 ++++++++++--
 lib/ext2fs/hashmap.h      |  4 ++--
 4 files changed, 29 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..818f7f05 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));
+				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);
+					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,
-- 

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

[Theodore Ts'o]

On Mon, May 31, 2021 at 09:23:49AM +0800, Wu Guanghao wrote:
> V1 -> V2:
> 
> [PATCH V2 03/12] zap_sector: fix memory leak
> 	free and return operators placed in {} block
> 
> [PATCH V2 04/12] ss_add_info_dir: fix memory leak and check whether,NULL pointer
> 	modified "=" to "=="
> 
> [PATCH V2 06/12] append_pathname: check the value returned by realloc to avoid segfault
> [PATCH V2 07/12] argv_parse: check return value of malloc in argv_parse()
> 	Fix typos
> 
> [PATCH V2 10/12] hashmap: change return value type of, ext2fs_hashmap_add()
> 	remove "new_block = NULL;"

Did you only send the patches that you changed, and didn't resend the
patches that didn't change between V1 and V2?

It's actually better if you resend the whole series in the future.

Thanks,

					- Ted

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

[Wu Guanghao]

Hello Ted,

Thank you for your advice, I will pay attention to it in the future.
Do I need to resend the series of patches
or continue to send the remaining patches this time?

Thanks,
Wu Guanghao

On Mon, 31 May 2021 00:28:46 -0400, Theodore Ts'o wrote:
> 
> 
> 
> On Mon, May 31, 2021 at 09:23:49AM +0800, Wu Guanghao wrote:
>> V1 -> V2:
>>
>> [PATCH V2 03/12] zap_sector: fix memory leak
>>     free and return operators placed in {} block
>>
>> [PATCH V2 04/12] ss_add_info_dir: fix memory leak and check whether,NULL pointer
>>     modified "=" to "=="
>>
>> [PATCH V2 06/12] append_pathname: check the value returned by realloc to avoid segfault
>> [PATCH V2 07/12] argv_parse: check return value of malloc in argv_parse()
>>     Fix typos
>>
>> [PATCH V2 10/12] hashmap: change return value type of, ext2fs_hashmap_add()
>>     remove "new_block = NULL;"
> 
> Did you only send the patches that you changed, and didn't resend the
> patches that didn't change between V1 and V2?
> 
> It's actually better if you resend the whole series in the future.
> 
> Thanks,
> 
>                     - Ted
> .
> .

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

[Zhiqiang Liu]

friendly ping...

What is the current status of the patch set？


On 2021/5/31 12:28, Theodore Ts'o wrote:
> On Mon, May 31, 2021 at 09:23:49AM +0800, Wu Guanghao wrote:
>> V1 -> V2:
>>
>> [PATCH V2 03/12] zap_sector: fix memory leak
>> 	free and return operators placed in {} block
>>
>> [PATCH V2 04/12] ss_add_info_dir: fix memory leak and check whether,NULL pointer
>> 	modified "=" to "=="
>>
>> [PATCH V2 06/12] append_pathname: check the value returned by realloc to avoid segfault
>> [PATCH V2 07/12] argv_parse: check return value of malloc in argv_parse()
>> 	Fix typos
>>
>> [PATCH V2 10/12] hashmap: change return value type of, ext2fs_hashmap_add()
>> 	remove "new_block = NULL;"
> Did you only send the patches that you changed, and didn't resend the
> patches that didn't change between V1 and V2?
>
> It's actually better if you resend the whole series in the future.
>
> Thanks,
>
> 					- Ted
>
> .