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

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

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 <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
Reviewed-by: Wu Bo <redacted>
---
 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;

-- 

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

Signed-off-by: Wu Guanghao <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
Reviewed-by: Wu Bo <redacted>
---
 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;
 	}

-- 

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

Signed-off-by: Wu Guanghao <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
Reviewed-by: Wu Bo <redacted>
---
 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;
 		}
 	}

-- 

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 <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
Reviewed-by: Wu Bo <redacted>
---
 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;
 }

-- 

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

Signed-off-by: Wu Guanghao <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
---
 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

-- 

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 <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
---
 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;
 }

-- 

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 <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
 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)

-- 

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 <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
 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++;
 	}

-- 

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

Signed-off-by: Zhiqiang Liu <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
 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)

-- 

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 <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
 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,

-- 

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

Signed-off-by: Zhiqiang Liu <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
 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);

-- 

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

Signed-off-by: Zhiqiang Liu <redacted>
Signed-off-by: Wu Guanghao <redacted>
Reviewed-by: Wu Bo <redacted>
---
 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);
 }

-- 

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.

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.

quoted

On 24 May 2021, at 14:20, Wu Guanghao [off-list ref] wrote:

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

Signed-off-by: Wu Guanghao <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
Reviewed-by: Wu Bo <redacted>
---
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;
		}
	}
-- 

.

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 写道:

Hello Wu,

Thanks for the patch.

Did you mean chdir() here?

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

Thanks,
Best regards,
Artem Blagodarenko.

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.

quoted

On 24 May 2021, at 14:23, Wu Guanghao [off-list ref] 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 <redacted>
Signed-off-by: Zhiqiang Liu <redacted>
---
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?

quoted

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

Thanks,
Best regards,
Artem Blagodarenko.

.

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.

Best regards,
Artem Blagodarenko

On 2021/5/25 20:45, Благодаренко Артём wrote:

quoted

On 24 May 2021, at 14:25, Wu Guanghao [off-list ref] 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 <redacted>
Signed-off-by: Wu Guanghao <redacted>
---
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.

quoted

+					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


.