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;
--
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..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
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..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
From: Zhiqiang Liu <[email protected]>
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);
}
--
2.19.1
From: Zhiqiang Liu <[email protected]>
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,
--
2.19.1
From: Zhiqiang Liu <[email protected]>
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..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
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 <[email protected]>
> Signed-off-by: Zhiqiang Liu <[email protected]>
> Reviewed-by: Wu Bo <[email protected]>
Applied, thanks.
- Ted
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 <[email protected]>
> Signed-off-by: Zhiqiang Liu <[email protected]>
> Reviewed-by: Wu Bo <[email protected]>
Thanks, applied.
- Ted
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 <[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..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
On Wed, Jun 30, 2021 at 04:27:19PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <[email protected]>
>
> In argv_parse(), return value of malloc should be checked
> whether it is NULL, otherwise, it may cause a segfault error.
Thanks, applied.
- Ted
On Wed, Jun 30, 2021 at 04:27:22PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <[email protected]>
>
> 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
On Wed, Jun 30, 2021 at 04:27:24PM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <[email protected]>
>
> In dupstr(), we should check return value of malloc().
Thanks, applied.
- Ted
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 <[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..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.
>
> .