From: Wu Guanghao <[email protected]>
v2->v3
[PATCH v3 04/12]ss_add_info_dir: don't zap the info->info_dirs and check whether NULL pointer
[PATCH v3 05/12]ss_create_invocation: fix memory leak and check whether NULL pointer
[PATCH v3 10/12]hashmap: change return value type of ext2fs_hashmap_add()
[PATCH v3 11/12]misc/lsattr: check whether path is NULL in lsattr_dir_proc()
modify according to Ted's suggestion.
Other patches have been applied and not sent
v1->v2
error correction
Zhiqiang Liu (6):
argv_parse: check return value of malloc in argv_parse()
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()
Wu Guanghao (6):
append_pathname: check the value returned by realloc
profile_create_node: set magic before strdup(name) to avoid memory
leak
tdb_transaction_recover: fix memory leak
zap_sector: fix memory leak
ss_add_info_dir: don't zap the info->info_dirs and check whether NULL pointer
ss_create_invocation: fix memory leak and check whether NULL pointer
contrib/android/base_fs.c | 14 +++++++++----
contrib/fsstress.c | 10 ++++++++--
ext2ed/main.c | 2 ++
lib/ext2fs/fileio.c | 11 ++++++++--
lib/ext2fs/hashmap.c | 12 +++++++++--
lib/ext2fs/hashmap.h | 5 +++--
lib/ext2fs/tdb.c | 1 +
lib/ss/error.c | 2 ++
lib/ss/help.c | 5 ++++-
lib/ss/invocation.c | 42 ++++++++++++++++++++++++++++++---------
lib/support/argv_parse.c | 2 ++
lib/support/profile.c | 3 ++-
misc/create_inode.c | 3 +++
misc/lsattr.c | 5 +++++
misc/mke2fs.c | 4 +++-
15 files changed, 97 insertions(+), 24 deletions(-)
--
2.27.0
From: Wu Guanghao <[email protected]>
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 | 42 +++++++++++++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/lib/ss/invocation.c b/lib/ss/invocation.c
index 457e7a2c..e458b785 100644
--- a/lib/ss/invocation.c
+++ b/lib/ss/invocation.c
@@ -26,29 +26,33 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
void *info_ptr, ss_request_table *request_table_ptr,
int *code_ptr)
{
- register int sci_idx;
- register ss_data *new_table;
- register ss_data **table;
+ int sci_idx;
+ ss_data *new_table = NULL;
+ ss_data **table = NULL;
+ ss_data **realloc_table = NULL;
*code_ptr = 0;
table = _ss_table;
new_table = (ss_data *) malloc(sizeof(ss_data));
+ if (!new_table)
+ goto out;
if (table == (ss_data **) NULL) {
table = (ss_data **) malloc(2 * size);
+ if (!table)
+ goto out;
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 out;
+
+ table = realloc_table;
table[sci_idx+1] = (ss_data *) NULL;
table[sci_idx] = new_table;
@@ -57,9 +61,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)
+ goto out;
+
*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)
+ goto out;
+
strcpy(new_table->prompt, subsystem_name);
strcat(new_table->prompt, ": ");
#ifdef silly
@@ -71,6 +81,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)
+ goto out;
+
*(new_table->rqt_tables) = request_table_ptr;
*(new_table->rqt_tables+1) = (ss_request_table *) NULL;
@@ -85,6 +98,17 @@ int ss_create_invocation(const char *subsystem_name, const char *version_string,
ss_get_readline(sci_idx);
#endif
return(sci_idx);
+
+out:
+ if (new_table) {
+ free(new_table->prompt);
+ free(new_table->info_dirs);
+ }
+ free(new_table);
+ free(table);
+ *code_ptr = ENOMEM;
+ return 0;
+
}
void
--
2.27.0
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 | 14 ++++++++++----
lib/ext2fs/fileio.c | 10 ++++++++--
lib/ext2fs/hashmap.c | 12 ++++++++++--
lib/ext2fs/hashmap.h | 5 +++--
4 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/contrib/android/base_fs.c b/contrib/android/base_fs.c
index 652317e2..4ed44169 100644
--- a/contrib/android/base_fs.c
+++ b/contrib/android/base_fs.c
@@ -100,7 +100,7 @@ static void free_base_fs_entry(void *e)
struct ext2fs_hashmap *basefs_parse(const char *file, const char *mountpoint)
{
- int err;
+ errcode_t err;
struct ext2fs_hashmap *entries = NULL;
struct basefs_entry *entry;
FILE *f = basefs_open(file);
@@ -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..2b0fb91e 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));
+ errcode_t 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..5239c921 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)
+errcode_t 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..43669a42 100644
--- a/lib/ext2fs/hashmap.h
+++ b/lib/ext2fs/hashmap.h
@@ -3,6 +3,7 @@
# include <stdlib.h>
# include <stdint.h>
+# include "et/com_err.h"
#ifndef __GNUC_PREREQ
#if defined(__GNUC__) && defined(__GNUC_MINOR__)
@@ -27,8 +28,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);
+errcode_t 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.27.0
From: Wu Guanghao <[email protected]>
In ss_add_info_dir(), don't zap the info->info_dirs. 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, 4 insertions(+), 1 deletion(-)
diff --git a/lib/ss/help.c b/lib/ss/help.c
index 5204401b..b4465bfe 100644
--- a/lib/ss/help.c
+++ b/lib/ss/help.c
@@ -148,13 +148,16 @@ 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) {
- info->info_dirs = (char **)NULL;
*code_ptr = errno;
return;
}
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.27.0
From: Zhiqiang Liu <[email protected]>
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 | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/misc/lsattr.c b/misc/lsattr.c
index 0d954376..7958f0ad 100644
--- a/misc/lsattr.c
+++ b/misc/lsattr.c
@@ -144,6 +144,11 @@ 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) {
+ fputs(_("Couldn't allocate path variable in lsattr_dir_proc"),
+ stderr);
+ return -1;
+ }
if (dir_len && dir_name[dir_len-1] == '/')
sprintf (path, "%s%s", dir_name, de->d_name);
--
2.27.0
On Wed, Jul 28, 2021 at 09:56:45AM +0800, wuguanghao wrote:
> From: Wu Guanghao <[email protected]>
>
> In ss_add_info_dir(), don't zap the info->info_dirs. 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]>
Thanks, applied.
- Ted
On Wed, Jul 28, 2021 at 09:56:46AM +0800, wuguanghao wrote:
> From: Wu Guanghao <[email protected]>
>
> 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]>
Thanks, applied.
- Ted
On Wed, Jul 28, 2021 at 09:56:47AM +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().
>
> Signed-off-by: Zhiqiang Liu <[email protected]>
> Signed-off-by: Wu Guanghao <[email protected]>
Thanks, applied.
Note: I changed ext2fs_hashmap_add() to return an int instead of an
errocode_t. The commit description said it was going to be an int,
and the code returns -1 (so I fixed the commit description to reflect
-1). Note that errcode_t is not appropriate for non-errno / com_err
error codes. So making the function prototype of hashmap_add() to
return an int is the correct thing to do.
Cheers,
- Ted
On Wed, Jul 28, 2021 at 09:56:48AM +0800, wuguanghao wrote:
> From: Zhiqiang Liu <[email protected]>
>
> 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]>
Thanks, applied.
Note that fputs() does not print a trailing newline (unlike puts()).
So I fixed up the error message to include a newline character at the
end.
- Ted