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