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 <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
Reviewed-by: Wu Bo <[email protected]>
---
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 zap_sector(), need free buf before return,
otherwise it will cause memory leak.
Signed-off-by: Wu Guanghao <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
Reviewed-by: Wu Bo <[email protected]>
---
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_create_invocation(), it is necessary to check whether
returned by malloc is a null pointer.
Signed-off-by: Wu Guanghao <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
---
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 tdb_transaction_recover(), need free data before return,
otherwise it will cause memory leak.
Signed-off-by: Wu Guanghao <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
Reviewed-by: Wu Bo <[email protected]>
---
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 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 <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
Reviewed-by: Wu Bo <[email protected]>
---
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 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 <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
---
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 ss_name(), we should check return value of malloc(),
otherwise, it may cause a segmentation fault problem.
Signed-off-by: Zhiqiang Liu <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
---
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 <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
---
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 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 <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
---
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 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 <[email protected]>
Signed-off-by: Zhiqiang Liu <[email protected]>
---
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 lsattr_dir_proc(), if malloc() return NULL, it will cause
a segmentation fault problem.
Signed-off-by: Zhiqiang Liu <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
---
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 <[email protected]>
Signed-off-by: Wu Guanghao <[email protected]>
Reviewed-by: Wu Bo <[email protected]>
---
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.
> On 24 May 2021, at 14:20, Wu Guanghao <[email protected]> wrote:
>
> In zap_sector(), need free buf before return,
> otherwise it will cause memory leak.
>
> Signed-off-by: Wu Guanghao <[email protected]>
> Signed-off-by: Zhiqiang Liu <[email protected]>
> Reviewed-by: Wu Bo <[email protected]>
> ---
> 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;
> }
> }
> --
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 <[email protected]> wrote:
>>
>> In zap_sector(), need free buf before return,
>> otherwise it will cause memory leak.
>>
>> Signed-off-by: Wu Guanghao <[email protected]>
>> Signed-off-by: Zhiqiang Liu <[email protected]>
>> Reviewed-by: Wu Bo <[email protected]>
>> ---
>> 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 д??:
> 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 <[email protected]>
> Signed-off-by: Wu Guanghao <[email protected]>
> ---
> 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)
>
Hello Wu,
Thanks for the patch.
> On 24 May 2021, at 14:23, Wu Guanghao <[email protected]> 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 <[email protected]>
> Signed-off-by: Zhiqiang Liu <[email protected]>
> ---
> 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.
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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Zhiqiang Liu <[email protected]>
>> ---
>> 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.
>
> .
>
> On 24 May 2021, at 14:25, Wu Guanghao <[email protected]> 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 <[email protected]>
> Signed-off-by: Wu Guanghao <[email protected]>
> ---
> 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
On 2021/5/25 20:45, Благодаренко Артём wrote:
>
>> On 24 May 2021, at 14:25, Wu Guanghao <[email protected]> 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 <[email protected]>
>> Signed-off-by: Wu Guanghao <[email protected]>
>> ---
>> 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
>
>
> .