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

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

[wuguanghao]

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;
-- 
2.19.1

[PATCH v2 02/12] tdb_transaction_recover: fix memory leak

[wuguanghao]

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..0fb94815 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;
 	}
 
-- 
2.19.1

Re: [PATCH v2 02/12] tdb_transaction_recover: fix memory leak

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:14PM +0800, wuguanghao wrote:
> 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>

Thanks, applied.

				- Ted

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

[wuguanghao]

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..68e36ecc 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;
+			}
 		}
 	}
 
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:15PM +0800, wuguanghao 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>

Thanks, applied.

					- Ted

[PATCH v2 04/12] ss_add_info_dir: fix memory leak and check whether

[wuguanghao]

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..6768b9b1 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;
 }
-- 
2.19.1

Re: [PATCH v2 04/12] ss_add_info_dir: fix memory leak and check whether

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:16PM +0800, wuguanghao wrote:
> 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..6768b9b1 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;

Adding the free() isn't right fix.  The real problem is that this line
should be removed:

  	info->info_dirs = (char **)NULL;

The function is trying to add a string (a directory name) to list, and
the realloc() is trying to extend the list.  If the realloc fils, we
shouldn't be zapping the original list.  We should just be returning,
and leaving the original list of directories unchanged and untouched.

    	    		      	 	     - Ted

Re: [PATCH v2 04/12] ss_add_info_dir: fix memory leak and check whether

[Zhiqiang Liu]

On 2021/7/16 11:27, Theodore Y. Ts'o wrote:
> On Wed, Jun 30, 2021 at 04:27:16PM +0800, wuguanghao wrote:
>> 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..6768b9b1 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;
> Adding the free() isn't right fix.  The real problem is that this line
> should be removed:
>
>   	info->info_dirs = (char **)NULL;
>
> The function is trying to add a string (a directory name) to list, and
> the realloc() is trying to extend the list.  If the realloc fils, we
> shouldn't be zapping the original list.  We should just be returning,
> and leaving the original list of directories unchanged and untouched.
>
>     	    		      	 	     - Ted

Thanks for your patient reply.

We will correct that in v3 patch.

>
> .

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

[wuguanghao]

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
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:17PM +0800, wuguanghao wrote:
> 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(-)
> 

Instead of adding all of these goto targets (which is fragile if for
some reason the code gets rearranged), it would be better to make sure
everything that we might want to free is initialized, i.e.:

  	register ss_data *new_table = NULL;
  	register ss_data **table = NULL;

  	new_table = (ss_data *) malloc(sizeof(ss_data));
	if (!new_table)
		goto out;
	memset(new_table, 0, sizeof(ss_data));

and then exit path can just look like this:

out:
	if (new_table) {
		free(new_table->prompt);
		free(new_table->info_dirs);
	}
	free(new_table);
	free(table);
	*code_ptr = ENOMEM;
	return 0;

... which is much cleaner, and means in the future, you don't need to
figure out which goto label you might need to jump to.

Cheers,

					- Ted

P.S.  And if we are making all of these changes to the function's
initializers, it might be a good time to zap the "register" keywords
for any lines we are changing, or are nearby, while we're at it.

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

[Zhiqiang Liu]

On 2021/7/16 11:34, Theodore Y. Ts'o wrote:
> On Wed, Jun 30, 2021 at 04:27:17PM +0800, wuguanghao wrote:
>> 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(-)
>>
> Instead of adding all of these goto targets (which is fragile if for
> some reason the code gets rearranged), it would be better to make sure
> everything that we might want to free is initialized, i.e.:
>
>   	register ss_data *new_table = NULL;
>   	register ss_data **table = NULL;
>
>   	new_table = (ss_data *) malloc(sizeof(ss_data));
> 	if (!new_table)
> 		goto out;
> 	memset(new_table, 0, sizeof(ss_data));
>
> and then exit path can just look like this:
>
> out:
> 	if (new_table) {
> 		free(new_table->prompt);
> 		free(new_table->info_dirs);
> 	}
> 	free(new_table);
> 	free(table);
> 	*code_ptr = ENOMEM;
> 	return 0;
>
> ... which is much cleaner, and means in the future, you don't need to
> figure out which goto label you might need to jump to.
>
> Cheers,
>
> 					- Ted
>
> P.S.  And if we are making all of these changes to the function's
> initializers, it might be a good time to zap the "register" keywords
> for any lines we are changing, or are nearby, while we're at it.
>
> .

Thanks for your suggestion.

We will rewrite the patch and remove the "register" keywords.


Regards

Zhiqiang Liu

.

[PATCH v2 06/12] append_pathname: check the value returned by realloc

[wuguanghao]

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..07433205 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;
 }
-- 
2.19.1

Re: [PATCH v2 06/12] append_pathname: check the value returned by realloc

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:18PM +0800, wuguanghao 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>

Thanks, applied, with a minor whitespace fixup, and the commit
description linewrapped.

					- Ted

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

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

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..1f50f9e5 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)
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:19PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In argv_parse(), return value of malloc should be checked
> whether it is NULL, otherwise, it may cause a segfault error.

Thanks, applied.

					- Ted

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

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

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++;
 	}
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:20PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> 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>

Thanks, applied.

					- Ted

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

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

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)
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:21PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In ss_name(), we should check return value of malloc(),
> otherwise, it may cause a segmentation fault problem.

Thanks, applied.

					- Ted

[PATCH v2 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 | 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,
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:22PM +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().

Changing the function signature of functions in libext2fs, which can
be a shared library, is problematic, since it can potentially break
callers who are depending on the existing shared library ABI.

In this case, making a function returning void return something else
isn't quite so bad, but it still puts callers in a quandry, since they
won't necessarily know if they have linked against an older library
which is not returning an error (or not).

Unfortunately, there's no other way to fix this, and creating a new
ext2fs_hashmap_add2() just to add a return code seems like overkill.
Grumble.

I guess it's OK to do it, since there _probably_ aren't users of
ext2fs_hashmap_add outside of e2fsprogs.  But if we are going to make
this change, we should really have ext2fs_hashmap_add return a
errcode_t, like the other libext2fs functions.

						- Ted

[PATCH v2 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 | 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);
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:23PM +0800, wuguanghao wrote:
> 
> 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;
> +	}

The string is missing a closing newline.  Also, why not?

		fputs(_("Couldn't allocate path variable in lsattr_dir_proc"),
		      stderr);

					- Ted

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

[Zhiqiang Liu]

On 2021/7/16 11:58, Theodore Y. Ts'o wrote:
> On Wed, Jun 30, 2021 at 04:27:23PM +0800, wuguanghao wrote:
>> 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;
>> +	}
> The string is missing a closing newline.  Also, why not?
>
> 		fputs(_("Couldn't allocate path variable in lsattr_dir_proc"),
> 		      stderr);
>
> 					- Ted
>
> .

Thanks for your suggestion.

We will resend the v3 patch as your suggestion.

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

[wuguanghao]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

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);
 }
-- 
2.19.1

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:24PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In dupstr(), we should check return value of malloc().

Thanks, applied.

					- Ted

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

[Theodore Y. Ts'o]

On Wed, Jun 30, 2021 at 04:27:13PM +0800, wuguanghao wrote:
> 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>

Applied, thanks.

					- Ted