[PATCH v2 00/28] fs/squashfs: fix memory leaks and introduce exists() function
This patch series fix several memory leaks, some use of dangling pointers (leading to cpu freeze) and finally introduce the exists() function for squashfs. This function enable testing the existence of a file, which is mandatory for using the distro_bootcmd Those fixes have been cut into several patches to be easier to review
Changes since v1: - patch 5: *dir = *file = NULL; is split in 2 lines - For consistency, sqfs_frag_lookup is modified to use a single "goto out" ( cf https://lists.denx.de/pipermail/u-boot/2020-October/429645.html ) - more memory leak fixes in sqfs_get_abs_path, sqfs_read and sqfs_probe - a missing error check typo in sqfs_get_abs_path - some missing reseting ctxt.sblk to NULL to prevent double free - reset cur_dev/cur_part_info to NULL when they are freed - return value of sqfs_decompressor_init() wasn't used - use "len" in sqfs_read to prevent writing beyond buffer - prevent reading with an offset since it doesn't work - prevent reading fragmented files since it doesn't work
Richard Genoud (28): fs/squashfs: fix board hang-up when calling .exists() fs/squashfs: sqfs_opendir: fix some memory leaks and dangling pointers fs/squashfs: sqfs_opendir: simplify error handling fs/squashfs: sqfs_closedir: fix memory leak fs/squashfs: sqfs_split_path: fix memory leak and dangling pointers fs/squashfs: sqfs_read_directory_table: fix memory leak fs/squashfs: sqfs_search_dir: fix dangling pointer fs/squashfs: sqfs_search_dir: fix memory leaks fs/squashfs: sqfs_read_inode_table: fix dangling pointer fs/squashfs: sqfs_concat_tokens: check if malloc succeeds fs/squashfs: sqfs_size: fix dangling pointer dirs->entry fs/squashfs: sqfs_size: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix dangling pointer dirs->entry fs/squashfs: sqfs_read: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix memory leak fs/squashfs: sqfs_read: fix another memory leak fs/squashfs: sqfs_frag_lookup: simplify error handling fs/squashfs: sqfs_get_abs_path: fix error check fs/squashfs: sqfs_get_abs_path: fix possible memory leak on error fs/squashfs: sqfs_read: fix memory leak on finfo.blk_sizes fs/squashfs: sqfs_probe: fix possible memory leak on error fs/squashfs: sqfs_close/sqfs_read_sblk: set ctxt.sblk to NULL after free fs/squashfs: sqfs_probe: reset cur_dev/cur_part_info to NULL on error fs/squashfs: sqfs_probe: use sqfs_decompressor_init() return value fs/squashfs: sqfs_read: don't write beyond buffer size fs/squashfs: sqfs_read: remove buggy offset functionality fs/squashfs: sqfs_read: fragmented files are not supported fs/squashfs: implement exists() function
fs/fs.c | 7 + fs/squashfs/sqfs.c | 399 +++++++++++++++++++++++++++++++-------------- include/squashfs.h | 1 + 3 files changed, 286 insertions(+), 121 deletions(-)
add missing squashfs function to prevent dangling or null pointers. For exemple, when calling test [ -e somefile ], squashfs.exists may be called.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/fs.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/fs/fs.c b/fs/fs.c index 29ad4d1a695..fb27c910d4f 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -287,6 +287,7 @@ static struct fstype_info fstypes[] = { { .fstype = FS_TYPE_SQUASHFS, .name = "squashfs", + .null_dev_desc_ok = false, .probe = sqfs_probe, .opendir = sqfs_opendir, .readdir = sqfs_readdir, @@ -295,6 +296,12 @@ static struct fstype_info fstypes[] = { .size = sqfs_size, .close = sqfs_close, .closedir = sqfs_closedir, + .exists = fs_exists_unsupported, + .uuid = fs_uuid_unsupported, + .write = fs_write_unsupported, + .ln = fs_ln_unsupported, + .unlink = fs_unlink_unsupported, + .mkdir = fs_mkdir_unsupported, }, #endif {
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
add missing squashfs function to prevent dangling or null pointers. For exemple, when calling test [ -e somefile ], squashfs.exists may be called.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/fs.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/fs/fs.c b/fs/fs.c index 29ad4d1a695..fb27c910d4f 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -287,6 +287,7 @@ static struct fstype_info fstypes[] = { { .fstype = FS_TYPE_SQUASHFS, .name = "squashfs",
.null_dev_desc_ok = false, .probe = sqfs_probe, .opendir = sqfs_opendir, .readdir = sqfs_readdir,@@ -295,6 +296,12 @@ static struct fstype_info fstypes[] = { .size = sqfs_size, .close = sqfs_close, .closedir = sqfs_closedir,
.exists = fs_exists_unsupported,.uuid = fs_uuid_unsupported,.write = fs_write_unsupported,.ln = fs_ln_unsupported,.unlink = fs_unlink_unsupported,.mkdir = fs_mkdir_unsupported, },#endif {
On Tue, Nov 03, 2020 at 12:10:59PM +0100, Richard Genoud wrote:
add missing squashfs function to prevent dangling or null pointers. For exemple, when calling test [ -e somefile ], squashfs.exists may be called.
Signed-off-by: Richard Genoud richard.genoud@posteo.net Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com
Applied to u-boot/master, thanks!
When trying to load an non-existing file, the cpu hangs!
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 15208b4dab0..1fdb9ac534b 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -821,22 +821,37 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) if (!dirs) return -EINVAL;
+ /* these should be set to NULL to prevent dangling pointers */ + dirs->dir_header = NULL; + dirs->entry = NULL; + dirs->table = NULL; + dirs->inode_table = NULL; + dirs->dir_table = NULL; + ret = sqfs_read_inode_table(&inode_table); - if (ret) - return -EINVAL; + if (ret) { + ret = -EINVAL; + goto free_dirs; + }
metablks_count = sqfs_read_directory_table(&dir_table, &pos_list); - if (metablks_count < 1) - return -EINVAL; + if (metablks_count < 1) { + ret = -EINVAL; + goto free_inode_table; + }
/* Tokenize filename */ token_count = sqfs_count_tokens(filename); - if (token_count < 0) - return -EINVAL; + if (token_count < 0) { + ret = -EINVAL; + goto free_inode_table; + }
path = strdup(filename); - if (!path) - return -ENOMEM; + if (!path) { + ret = -EINVAL; + goto free_inode_table; + }
token_list = malloc(token_count * sizeof(char *)); if (!token_list) { @@ -882,6 +897,12 @@ free_tokens: free(pos_list); free_path: free(path); +free_inode_table: + if (ret) + free(inode_table); +free_dirs: + if (ret) + free(dirs);
return ret; }
On Tue, Nov 03, 2020 at 12:11:00PM +0100, Richard Genoud wrote:
When trying to load an non-existing file, the cpu hangs!
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
Using only one label permits to prevents bugs when moving code around.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1fdb9ac534b..b94a9715205 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -812,9 +812,9 @@ free_dtb: int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) { unsigned char *inode_table = NULL, *dir_table = NULL; - int j, token_count, ret = 0, metablks_count; + int j, token_count = 0, ret = 0, metablks_count; struct squashfs_dir_stream *dirs; - char **token_list, *path; + char **token_list = NULL, *path = NULL; u32 *pos_list = NULL;
dirs = malloc(sizeof(*dirs)); @@ -831,38 +831,38 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) ret = sqfs_read_inode_table(&inode_table); if (ret) { ret = -EINVAL; - goto free_dirs; + goto out; }
metablks_count = sqfs_read_directory_table(&dir_table, &pos_list); if (metablks_count < 1) { ret = -EINVAL; - goto free_inode_table; + goto out; }
/* Tokenize filename */ token_count = sqfs_count_tokens(filename); if (token_count < 0) { ret = -EINVAL; - goto free_inode_table; + goto out; }
path = strdup(filename); if (!path) { ret = -EINVAL; - goto free_inode_table; + goto out; }
token_list = malloc(token_count * sizeof(char *)); if (!token_list) { ret = -EINVAL; - goto free_path; + goto out; }
/* Fill tokens list */ ret = sqfs_tokenize(token_list, token_count, path); if (ret) - goto free_tokens; + goto out; /* * ldir's (extended directory) size is greater than dir, so it works as * a general solution for the malloc size, since 'i' is a union. @@ -872,7 +872,7 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) ret = sqfs_search_dir(dirs, token_list, token_count, pos_list, metablks_count); if (ret) - goto free_tokens; + goto out;
if (le16_to_cpu(dirs->i_dir.inode_type) == SQFS_DIR_TYPE) dirs->size = le16_to_cpu(dirs->i_dir.file_size); @@ -890,19 +890,16 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp)
*dirsp = (struct fs_dir_stream *)dirs;
-free_tokens: +out: for (j = 0; j < token_count; j++) free(token_list[j]); free(token_list); free(pos_list); -free_path: free(path); -free_inode_table: - if (ret) + if (ret) { free(inode_table); -free_dirs: - if (ret) free(dirs); + }
return ret; }
I am not sure if this simplification follows the most commonly used strategy for handling errors (in U-Boot's source code), but it does not bother me either. Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
Using only one label permits to prevents bugs when moving code around.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1fdb9ac534b..b94a9715205 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -812,9 +812,9 @@ free_dtb: int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) { unsigned char *inode_table = NULL, *dir_table = NULL;
int j, token_count, ret = 0, metablks_count;
int j, token_count = 0, ret = 0, metablks_count; struct squashfs_dir_stream *dirs;
char **token_list, *path;
char **token_list = NULL, *path = NULL; u32 *pos_list = NULL; dirs = malloc(sizeof(*dirs));@@ -831,38 +831,38 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) ret = sqfs_read_inode_table(&inode_table); if (ret) { ret = -EINVAL;
goto free_dirs;
goto out; } metablks_count = sqfs_read_directory_table(&dir_table, &pos_list); if (metablks_count < 1) { ret = -EINVAL;
goto free_inode_table;
goto out; } /* Tokenize filename */ token_count = sqfs_count_tokens(filename); if (token_count < 0) { ret = -EINVAL;
goto free_inode_table;
goto out; } path = strdup(filename); if (!path) { ret = -EINVAL;
goto free_inode_table;
goto out; } token_list = malloc(token_count * sizeof(char *)); if (!token_list) { ret = -EINVAL;
goto free_path;
goto out; } /* Fill tokens list */ ret = sqfs_tokenize(token_list, token_count, path); if (ret)
goto free_tokens;
goto out; /* * ldir's (extended directory) size is greater than dir, so itworks as * a general solution for the malloc size, since 'i' is a union. @@ -872,7 +872,7 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) ret = sqfs_search_dir(dirs, token_list, token_count, pos_list, metablks_count); if (ret)
goto free_tokens;
goto out; if (le16_to_cpu(dirs->i_dir.inode_type) == SQFS_DIR_TYPE) dirs->size = le16_to_cpu(dirs->i_dir.file_size);@@ -890,19 +890,16 @@ int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp)
*dirsp = (struct fs_dir_stream *)dirs;-free_tokens: +out: for (j = 0; j < token_count; j++) free(token_list[j]); free(token_list); free(pos_list); -free_path: free(path); -free_inode_table:
if (ret)
if (ret) { free(inode_table);-free_dirs:
if (ret) free(dirs);
} return ret;}
On Tue, Nov 03, 2020 at 12:11:01PM +0100, Richard Genoud wrote:
Using only one label permits to prevents bugs when moving code around.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
sqfs_dirs wasn't freed anywhere.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index b94a9715205..0ac922af9e7 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1573,4 +1573,5 @@ void sqfs_closedir(struct fs_dir_stream *dirs) free(sqfs_dirs->inode_table); free(sqfs_dirs->dir_table); free(sqfs_dirs->dir_header); + free(sqfs_dirs); }
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
sqfs_dirs wasn't freed anywhere.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index b94a9715205..0ac922af9e7 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1573,4 +1573,5 @@ void sqfs_closedir(struct fs_dir_stream *dirs) free(sqfs_dirs->inode_table); free(sqfs_dirs->dir_table); free(sqfs_dirs->dir_header);
free(sqfs_dirs);}
On Tue, Nov 03, 2020 at 12:11:02PM +0100, Richard Genoud wrote:
sqfs_dirs wasn't freed anywhere.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
*file and *dir were not freed on error
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 0ac922af9e7..58b8bfc66dc 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1089,15 +1089,27 @@ static int sqfs_split_path(char **file, char **dir, const char *path) char *dirc, *basec, *bname, *dname, *tmp_path; int ret = 0;
+ *file = NULL; + *dir = NULL; + dirc = NULL; + basec = NULL; + bname = NULL; + dname = NULL; + tmp_path = NULL; + /* check for first slash in path*/ if (path[0] == '/') { tmp_path = strdup(path); - if (!tmp_path) - return -ENOMEM; + if (!tmp_path) { + ret = -ENOMEM; + goto out; + } } else { tmp_path = malloc(strlen(path) + 2); - if (!tmp_path) - return -ENOMEM; + if (!tmp_path) { + ret = -ENOMEM; + goto out; + } tmp_path[0] = '/'; strcpy(tmp_path + 1, path); } @@ -1106,13 +1118,13 @@ static int sqfs_split_path(char **file, char **dir, const char *path) dirc = strdup(tmp_path); if (!dirc) { ret = -ENOMEM; - goto free_tmp; + goto out; }
basec = strdup(tmp_path); if (!basec) { ret = -ENOMEM; - goto free_dirc; + goto out; }
dname = sqfs_dirname(dirc); @@ -1122,14 +1134,14 @@ static int sqfs_split_path(char **file, char **dir, const char *path)
if (!*file) { ret = -ENOMEM; - goto free_basec; + goto out; }
if (*dname == '\0') { *dir = malloc(2); if (!*dir) { ret = -ENOMEM; - goto free_basec; + goto out; }
(*dir)[0] = '/'; @@ -1138,15 +1150,19 @@ static int sqfs_split_path(char **file, char **dir, const char *path) *dir = strdup(dname); if (!*dir) { ret = -ENOMEM; - goto free_basec; + goto out; } }
-free_basec: +out: + if (ret) { + free(*file); + free(*dir); + *dir = NULL; + *file = NULL; + } free(basec); -free_dirc: free(dirc); -free_tmp: free(tmp_path);
return ret;
Same comment about the error path simplification. Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
*file and *dir were not freed on error
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 0ac922af9e7..58b8bfc66dc 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1089,15 +1089,27 @@ static int sqfs_split_path(char **file, char **dir, const char *path) char *dirc, *basec, *bname, *dname, *tmp_path; int ret = 0;
*file = NULL;*dir = NULL;dirc = NULL;basec = NULL;bname = NULL;dname = NULL;tmp_path = NULL;/* check for first slash in path*/ if (path[0] == '/') { tmp_path = strdup(path);
if (!tmp_path)return -ENOMEM;
if (!tmp_path) {ret = -ENOMEM;goto out;} } else { tmp_path = malloc(strlen(path) + 2);
if (!tmp_path)return -ENOMEM;
if (!tmp_path) {ret = -ENOMEM;goto out;} tmp_path[0] = '/'; strcpy(tmp_path + 1, path); }@@ -1106,13 +1118,13 @@ static int sqfs_split_path(char **file, char **dir, const char *path) dirc = strdup(tmp_path); if (!dirc) { ret = -ENOMEM;
goto free_tmp;
goto out; } basec = strdup(tmp_path); if (!basec) { ret = -ENOMEM;
goto free_dirc;
goto out; } dname = sqfs_dirname(dirc);@@ -1122,14 +1134,14 @@ static int sqfs_split_path(char **file, char **dir, const char *path)
if (!*file) { ret = -ENOMEM;
goto free_basec;
goto out; } if (*dname == '\0') { *dir = malloc(2); if (!*dir) { ret = -ENOMEM;
goto free_basec;
goto out; } (*dir)[0] = '/';@@ -1138,15 +1150,19 @@ static int sqfs_split_path(char **file, char **dir, const char *path) *dir = strdup(dname); if (!*dir) { ret = -ENOMEM;
goto free_basec;
goto out; } }-free_basec: +out:
if (ret) {free(*file);free(*dir);*dir = NULL;*file = NULL;} free(basec);-free_dirc: free(dirc); -free_tmp: free(tmp_path);
return ret;
On Tue, Nov 03, 2020 at 12:11:03PM +0100, Richard Genoud wrote:
*file and *dir were not freed on error
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
pos_list wasn't freed on every error
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 58b8bfc66dc..9d460e8bed6 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -722,6 +722,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) unsigned long dest_len = 0; bool compressed;
+ *dir_table = NULL; + *pos_list = NULL; /* DIRECTORY TABLE */ table_size = get_unaligned_le64(&sblk->fragment_table_start) - get_unaligned_le64(&sblk->directory_table_start); @@ -736,35 +738,31 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) return -ENOMEM;
if (sqfs_disk_read(start, n_blks, dtb) < 0) - goto free_dtb; + goto out;
/* Parse directory table (metadata block) header */ ret = sqfs_read_metablock(dtb, table_offset, &compressed, &src_len); if (ret) - goto free_dtb; + goto out;
/* Calculate total size to store the whole decompressed table */ metablks_count = sqfs_count_metablks(dtb, table_offset, table_size); if (metablks_count < 1) - goto free_dtb; + goto out;
*dir_table = malloc(metablks_count * SQFS_METADATA_BLOCK_SIZE); if (!*dir_table) - goto free_dtb; + goto out;
*pos_list = malloc(metablks_count * sizeof(u32)); - if (!*pos_list) { - free(*dir_table); - goto free_dtb; - } + if (!*pos_list) + goto out;
ret = sqfs_get_metablk_pos(*pos_list, dtb, table_offset, metablks_count); if (ret) { metablks_count = -1; - free(*dir_table); - free(*pos_list); - goto free_dtb; + goto out; }
src_table = dtb + table_offset + SQFS_HEADER_SIZE; @@ -780,8 +778,7 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) &dest_len, src_table, src_len); if (ret) { metablks_count = -1; - free(*dir_table); - goto free_dtb; + goto out; }
if (dest_len < SQFS_METADATA_BLOCK_SIZE) { @@ -803,7 +800,13 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) src_table += src_len + SQFS_HEADER_SIZE; }
-free_dtb: +out: + if (metablks_count < 1) { + free(*dir_table); + free(*pos_list); + *dir_table = NULL; + *pos_list = NULL; + } free(dtb);
return metablks_count;
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
pos_list wasn't freed on every error
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 58b8bfc66dc..9d460e8bed6 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -722,6 +722,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) unsigned long dest_len = 0; bool compressed;
*dir_table = NULL;*pos_list = NULL; /* DIRECTORY TABLE */ table_size = get_unaligned_le64(&sblk->fragment_table_start) - get_unaligned_le64(&sblk->directory_table_start);@@ -736,35 +738,31 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) return -ENOMEM;
if (sqfs_disk_read(start, n_blks, dtb) < 0)
goto free_dtb;
goto out; /* Parse directory table (metadata block) header */ ret = sqfs_read_metablock(dtb, table_offset, &compressed,&src_len); if (ret)
goto free_dtb;
goto out; /* Calculate total size to store the whole decompressed table */ metablks_count = sqfs_count_metablks(dtb, table_offset,table_size); if (metablks_count < 1)
goto free_dtb;
goto out; *dir_table = malloc(metablks_count * SQFS_METADATA_BLOCK_SIZE); if (!*dir_table)
goto free_dtb;
goto out; *pos_list = malloc(metablks_count * sizeof(u32));
if (!*pos_list) {free(*dir_table);goto free_dtb;}
if (!*pos_list)goto out; ret = sqfs_get_metablk_pos(*pos_list, dtb, table_offset, metablks_count); if (ret) { metablks_count = -1;
free(*dir_table);free(*pos_list);goto free_dtb;
goto out; } src_table = dtb + table_offset + SQFS_HEADER_SIZE;@@ -780,8 +778,7 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) &dest_len, src_table, src_len); if (ret) { metablks_count = -1;
free(*dir_table);goto free_dtb;
goto out; } if (dest_len < SQFS_METADATA_BLOCK_SIZE) {@@ -803,7 +800,13 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) src_table += src_len + SQFS_HEADER_SIZE; }
-free_dtb: +out:
if (metablks_count < 1) {free(*dir_table);free(*pos_list);*dir_table = NULL;*pos_list = NULL;} free(dtb); return metablks_count;
On Tue, Nov 03, 2020 at 12:11:04PM +0100, Richard Genoud wrote:
pos_list wasn't freed on every error
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 9d460e8bed6..78893b5c85d 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -485,6 +485,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, if (!ret) break; free(dirs->entry); + dirs->entry = NULL; }
if (ret) { @@ -530,6 +531,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, if (ret) return -EINVAL; free(dirs->entry); + dirs->entry = NULL;
ret = sqfs_search_dir(dirs, sym_tokens, token_count, m_list, m_count); @@ -537,6 +539,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, } else if (!sqfs_is_dir(get_unaligned_le16(&dir->inode_type))) { printf("** Cannot find directory. **\n"); free(dirs->entry); + dirs->entry = NULL; return -EINVAL; }
@@ -556,6 +559,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, if (sqfs_is_empty_dir(table)) { printf("Empty directory.\n"); free(dirs->entry); + dirs->entry = NULL; return SQFS_EMPTY_DIR; }
@@ -564,6 +568,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, dirs->entry_count = dirs->dir_header->count + 1; dirs->size -= SQFS_DIR_HEADER_SIZE; free(dirs->entry); + dirs->entry = NULL; }
offset = sqfs_dir_offset(table, m_list, m_count);
On Tue, Nov 03, 2020 at 12:11:05PM +0100, Richard Genoud wrote:
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
path, target, res, rem and sym_tokens were not free on error nor success.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 64 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 13 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 78893b5c85d..1714306e747 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -434,7 +434,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, { struct squashfs_super_block *sblk = ctxt.sblk; char *path, *target, **sym_tokens, *res, *rem; - int j, ret, new_inode_number, offset; + int j, ret = 0, new_inode_number, offset; struct squashfs_symlink_inode *sym; struct squashfs_ldir_inode *ldir; struct squashfs_dir_inode *dir; @@ -442,6 +442,12 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, struct fs_dirent *dent; unsigned char *table;
+ res = NULL; + rem = NULL; + path = NULL; + target = NULL; + sym_tokens = NULL; + dirsp = (struct fs_dir_stream *)dirs;
/* Start by root inode */ @@ -477,7 +483,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, for (j = 0; j < token_count; j++) { if (!sqfs_is_dir(get_unaligned_le16(&dir->inode_type))) { printf("** Cannot find directory. **\n"); - return -EINVAL; + ret = -EINVAL; + goto out; }
while (!sqfs_readdir(dirsp, &dent)) { @@ -490,7 +497,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
if (ret) { printf("** Cannot find directory. **\n"); - return -EINVAL; + ret = -EINVAL; + goto out; }
/* Redefine inode as the found token */ @@ -507,40 +515,63 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, sym = (struct squashfs_symlink_inode *)table; /* Get first j + 1 tokens */ path = sqfs_concat_tokens(token_list, j + 1); + if (!path) { + ret = -ENOMEM; + goto out; + } /* Resolve for these tokens */ target = sqfs_resolve_symlink(sym, path); + if (!target) { + ret = -ENOMEM; + goto out; + } /* Join remaining tokens */ rem = sqfs_concat_tokens(token_list + j + 1, token_count - j - 1); + if (!rem) { + ret = -ENOMEM; + goto out; + } /* Concatenate remaining tokens and symlink's target */ res = malloc(strlen(rem) + strlen(target) + 1); + if (!res) { + ret = -ENOMEM; + goto out; + } strcpy(res, target); res[strlen(target)] = '/'; strcpy(res + strlen(target) + 1, rem); token_count = sqfs_count_tokens(res);
- if (token_count < 0) - return -EINVAL; + if (token_count < 0) { + ret = -EINVAL; + goto out; + }
sym_tokens = malloc(token_count * sizeof(char *)); - if (!sym_tokens) - return -EINVAL; + if (!sym_tokens) { + ret = -EINVAL; + goto out; + }
/* Fill tokens list */ ret = sqfs_tokenize(sym_tokens, token_count, res); - if (ret) - return -EINVAL; + if (ret) { + ret = -EINVAL; + goto out; + } free(dirs->entry); dirs->entry = NULL;
ret = sqfs_search_dir(dirs, sym_tokens, token_count, m_list, m_count); - return ret; + goto out; } else if (!sqfs_is_dir(get_unaligned_le16(&dir->inode_type))) { printf("** Cannot find directory. **\n"); free(dirs->entry); dirs->entry = NULL; - return -EINVAL; + ret = -EINVAL; + goto out; }
/* Check if it is an extended dir. */ @@ -560,7 +591,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, printf("Empty directory.\n"); free(dirs->entry); dirs->entry = NULL; - return SQFS_EMPTY_DIR; + ret = SQFS_EMPTY_DIR; + goto out; }
dirs->table += SQFS_DIR_HEADER_SIZE; @@ -579,7 +611,13 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, else memcpy(&dirs->i_ldir, ldir, sizeof(*ldir));
- return 0; +out: + free(res); + free(rem); + free(path); + free(target); + free(sym_tokens); + return ret; }
/*
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
path, target, res, rem and sym_tokens were not free on error nor success.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 64 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 13 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 78893b5c85d..1714306e747 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -434,7 +434,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, { struct squashfs_super_block *sblk = ctxt.sblk; char *path, *target, **sym_tokens, *res, *rem;
int j, ret, new_inode_number, offset;
int j, ret = 0, new_inode_number, offset; struct squashfs_symlink_inode *sym; struct squashfs_ldir_inode *ldir; struct squashfs_dir_inode *dir;@@ -442,6 +442,12 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, struct fs_dirent *dent; unsigned char *table;
res = NULL;rem = NULL;path = NULL;target = NULL;sym_tokens = NULL;dirsp = (struct fs_dir_stream *)dirs; /* Start by root inode */@@ -477,7 +483,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, for (j = 0; j < token_count; j++) { if (!sqfs_is_dir(get_unaligned_le16(&dir->inode_type))) { printf("** Cannot find directory. **\n");
return -EINVAL;
ret = -EINVAL;goto out; } while (!sqfs_readdir(dirsp, &dent)) {@@ -490,7 +497,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
if (ret) { printf("** Cannot find directory. **\n");
return -EINVAL;
ret = -EINVAL;goto out; } /* Redefine inode as the found token */@@ -507,40 +515,63 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, sym = (struct squashfs_symlink_inode *)table; /* Get first j + 1 tokens */ path = sqfs_concat_tokens(token_list, j + 1);
if (!path) {ret = -ENOMEM;goto out;} /* Resolve for these tokens */ target = sqfs_resolve_symlink(sym, path);if (!target) {ret = -ENOMEM;goto out;} /* Join remaining tokens */ rem = sqfs_concat_tokens(token_list + j + 1,token_count - j - 1);
if (!rem) {ret = -ENOMEM;goto out;} /* Concatenate remaining tokens and symlink'starget */ res = malloc(strlen(rem) + strlen(target) + 1);
if (!res) {ret = -ENOMEM;goto out;} strcpy(res, target); res[strlen(target)] = '/'; strcpy(res + strlen(target) + 1, rem); token_count = sqfs_count_tokens(res);
if (token_count < 0)return -EINVAL;
if (token_count < 0) {ret = -EINVAL;goto out;} sym_tokens = malloc(token_count * sizeof(char *));
if (!sym_tokens)return -EINVAL;
if (!sym_tokens) {ret = -EINVAL;goto out;} /* Fill tokens list */ ret = sqfs_tokenize(sym_tokens, token_count, res);
if (ret)return -EINVAL;
if (ret) {ret = -EINVAL;goto out;} free(dirs->entry); dirs->entry = NULL; ret = sqfs_search_dir(dirs, sym_tokens,token_count, m_list, m_count);
return ret;
goto out; } else if(!sqfs_is_dir(get_unaligned_le16(&dir->inode_type))) { printf("** Cannot find directory. **\n"); free(dirs->entry); dirs->entry = NULL;
return -EINVAL;
ret = -EINVAL;goto out; } /* Check if it is an extended dir. */@@ -560,7 +591,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, printf("Empty directory.\n"); free(dirs->entry); dirs->entry = NULL;
return SQFS_EMPTY_DIR;
ret = SQFS_EMPTY_DIR;goto out; } dirs->table += SQFS_DIR_HEADER_SIZE;@@ -579,7 +611,13 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, else memcpy(&dirs->i_ldir, ldir, sizeof(*ldir));
return 0;+out:
free(res);free(rem);free(path);free(target);free(sym_tokens);return ret;}
/*
On Tue, Nov 03, 2020 at 12:11:06PM +0100, Richard Genoud wrote:
path, target, res, rem and sym_tokens were not free on error nor success.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
inode_table should not be left dangling as it may be freed in sqfs_opendir
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1714306e747..72181f38332 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -731,6 +731,7 @@ static int sqfs_read_inode_table(unsigned char **inode_table) src_table, src_len); if (ret) { free(*inode_table); + *inode_table = NULL; goto free_itb; }
On Tue, Nov 03, 2020 at 12:11:07PM +0100, Richard Genoud wrote:
inode_table should not be left dangling as it may be freed in sqfs_opendir
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
memory allocation should always be checked
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 72181f38332..7da2e09cc36 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -242,6 +242,9 @@ static char *sqfs_concat_tokens(char **token_list, int token_count) length = sqfs_get_tokens_length(token_list, token_count);
result = malloc(length + 1); + if (!result) + return NULL; + result[length] = '\0';
for (i = 0; i < token_count; i++) {
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
memory allocation should always be checked
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 72181f38332..7da2e09cc36 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -242,6 +242,9 @@ static char *sqfs_concat_tokens(char **token_list, int token_count) length = sqfs_get_tokens_length(token_list, token_count);
result = malloc(length + 1);
if (!result)return NULL;result[length] = '\0'; for (i = 0; i < token_count; i++) {
On Tue, Nov 03, 2020 at 12:11:08PM +0100, Richard Genoud wrote:
memory allocation should always be checked
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 7da2e09cc36..3b008b5235c 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1569,6 +1569,7 @@ int sqfs_size(const char *filename, loff_t *size) if (!ret) break; free(dirs->entry); + dirs->entry = NULL; }
if (ret) { @@ -1582,6 +1583,7 @@ int sqfs_size(const char *filename, loff_t *size) ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes, sblk->block_size); free(dirs->entry); + dirs->entry = NULL;
base = (struct squashfs_base_inode *)ipos; switch (get_unaligned_le16(&base->inode_type)) {
On Tue, Nov 03, 2020 at 12:11:09PM +0100, Richard Genoud wrote:
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
as sqfs_opendir failed, there's no need to call sqfs_closedir
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 3b008b5235c..f4cac3e4bf0 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1557,7 +1557,6 @@ int sqfs_size(const char *filename, loff_t *size) */ ret = sqfs_opendir(dir, &dirsp); if (ret) { - sqfs_closedir(dirsp); ret = -EINVAL; goto free_strings; }
On Tue, Nov 03, 2020 at 12:11:10PM +0100, Richard Genoud wrote:
as sqfs_opendir failed, there's no need to call sqfs_closedir
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index f4cac3e4bf0..13e64bea934 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1336,6 +1336,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, break;
free(dirs->entry); + dirs->entry = NULL; }
if (ret) {
On Tue, Nov 03, 2020 at 12:11:11PM +0100, Richard Genoud wrote:
dirs->entry shouldn't be left dangling as it could be freed twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
as sqfs_opendir failed, there's no need to call sqfs_closedir
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 13e64bea934..1ac07625889 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1323,7 +1323,6 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sqfs_split_path(&file, &dir, filename); ret = sqfs_opendir(dir, &dirsp); if (ret) { - sqfs_closedir(dirsp); goto free_paths; }
On Tue, Nov 03, 2020 at 12:11:12PM +0100, Richard Genoud wrote:
as sqfs_opendir failed, there's no need to call sqfs_closedir
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
sqfs_closedir() should be called to free memory allocated by sqfs_opendir()
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1ac07625889..a9e803cbac2 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1341,7 +1341,6 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, if (ret) { printf("File not found.\n"); *actread = 0; - sqfs_closedir(dirsp); ret = -ENOENT; goto free_paths; } @@ -1532,6 +1531,7 @@ free_datablk: free_paths: free(file); free(dir); + sqfs_closedir(dirsp);
return ret; }
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
sqfs_closedir() should be called to free memory allocated by sqfs_opendir()
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1ac07625889..a9e803cbac2 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1341,7 +1341,6 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, if (ret) { printf("File not found.\n"); *actread = 0;
sqfs_closedir(dirsp); ret = -ENOENT; goto free_paths; }@@ -1532,6 +1531,7 @@ free_datablk: free_paths: free(file); free(dir);
sqfs_closedir(dirsp); return ret;}
On Tue, Nov 03, 2020 at 12:11:13PM +0100, Richard Genoud wrote:
sqfs_closedir() should be called to free memory allocated by sqfs_opendir()
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
data_buffer was allocated in a loop and freed only once.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index a9e803cbac2..cfea313e34b 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1459,6 +1459,8 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, }
data_offset += table_size; + free(data_buffer); + data_buffer = NULL; }
free(finfo.blk_sizes);
On Tue, Nov 03, 2020 at 12:11:14PM +0100, Richard Genoud wrote:
data_buffer was allocated in a loop and freed only once.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
For consistency with other functions.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index cfea313e34b..b97a961c5e3 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -106,6 +106,10 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, int block, offset, ret; u16 header;
+ metadata_buffer = NULL; + entries = NULL; + table = NULL; + if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) return -EINVAL;
@@ -117,12 +121,14 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
/* Allocate a proper sized buffer to store the fragment index table */ table = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); - if (!table) - return -ENOMEM; + if (!table) { + ret = -ENOMEM; + goto out; + }
if (sqfs_disk_read(start, n_blks, table) < 0) { - free(table); - return -EINVAL; + ret = -EINVAL; + goto out; }
block = SQFS_FRAGMENT_INDEX(inode_fragment_index); @@ -142,12 +148,12 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, metadata_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); if (!metadata_buffer) { ret = -ENOMEM; - goto free_table; + goto out; }
if (sqfs_disk_read(start, n_blks, metadata_buffer) < 0) { ret = -EINVAL; - goto free_buffer; + goto out; }
/* Every metadata block starts with a 16-bit header */ @@ -156,13 +162,13 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
if (!metadata || !header) { ret = -ENOMEM; - goto free_buffer; + goto out; }
entries = malloc(SQFS_METADATA_BLOCK_SIZE); if (!entries) { ret = -ENOMEM; - goto free_buffer; + goto out; }
if (SQFS_COMPRESSED_METADATA(header)) { @@ -172,7 +178,7 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, src_len); if (ret) { ret = -EINVAL; - goto free_entries; + goto out; } } else { memcpy(entries, metadata, SQFS_METADATA_SIZE(header)); @@ -181,11 +187,9 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, *e = entries[offset]; ret = SQFS_COMPRESSED_BLOCK(e->size);
-free_entries: +out: free(entries); -free_buffer: free(metadata_buffer); -free_table: free(table);
return ret;
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
For consistency with other functions.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index cfea313e34b..b97a961c5e3 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -106,6 +106,10 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, int block, offset, ret; u16 header;
metadata_buffer = NULL;entries = NULL;table = NULL;if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) return -EINVAL;@@ -117,12 +121,14 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
/* Allocate a proper sized buffer to store the fragment indextable */ table = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
if (!table)return -ENOMEM;
if (!table) {ret = -ENOMEM;goto out;} if (sqfs_disk_read(start, n_blks, table) < 0) {
free(table);return -EINVAL;
ret = -EINVAL;goto out; } block = SQFS_FRAGMENT_INDEX(inode_fragment_index);@@ -142,12 +148,12 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, metadata_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); if (!metadata_buffer) { ret = -ENOMEM;
goto free_table;
goto out; } if (sqfs_disk_read(start, n_blks, metadata_buffer) < 0) { ret = -EINVAL;
goto free_buffer;
goto out; } /* Every metadata block starts with a 16-bit header */@@ -156,13 +162,13 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
if (!metadata || !header) { ret = -ENOMEM;
goto free_buffer;
goto out; } entries = malloc(SQFS_METADATA_BLOCK_SIZE); if (!entries) { ret = -ENOMEM;
goto free_buffer;
goto out; } if (SQFS_COMPRESSED_METADATA(header)) {@@ -172,7 +178,7 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, src_len); if (ret) { ret = -EINVAL;
goto free_entries;
goto out; } } else { memcpy(entries, metadata, SQFS_METADATA_SIZE(header));@@ -181,11 +187,9 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, *e = entries[offset]; ret = SQFS_COMPRESSED_BLOCK(e->size);
-free_entries: +out: free(entries); -free_buffer: free(metadata_buffer); -free_table: free(table);
return ret;
On Tue, Nov 03, 2020 at 12:11:15PM +0100, Richard Genoud wrote:
For consistency with other functions.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
the return value of sqfs_tokenize(rel_tokens, rc, rel); wasn't checked. (but "ret" value was !) This is obviouly a typo.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index b97a961c5e3..825d5d13fa2 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -359,7 +359,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) if (ret) goto free_r_tokens;
- sqfs_tokenize(rel_tokens, rc, rel); + ret = sqfs_tokenize(rel_tokens, rc, rel); if (ret) goto free_r_tokens;
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
the return value of sqfs_tokenize(rel_tokens, rc, rel); wasn't checked. (but "ret" value was !) This is obviouly a typo.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index b97a961c5e3..825d5d13fa2 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -359,7 +359,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) if (ret) goto free_r_tokens;
sqfs_tokenize(rel_tokens, rc, rel);
ret = sqfs_tokenize(rel_tokens, rc, rel); if (ret) goto free_r_tokens;
On Tue, Nov 03, 2020 at 12:11:16PM +0100, Richard Genoud wrote:
the return value of sqfs_tokenize(rel_tokens, rc, rel); wasn't checked. (but "ret" value was !) This is obviouly a typo.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
if sqfs_tokenize(rel_tokens, rc, rel); fails, the function exits without freeing the array base_tokens.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 825d5d13fa2..f41deece0ae 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -340,28 +340,31 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) char **base_tokens, **rel_tokens, *resolved = NULL; int ret, bc, rc, i, updir = 0, resolved_size = 0, offset = 0;
+ base_tokens = NULL; + rel_tokens = NULL; + /* Memory allocation for the token lists */ bc = sqfs_count_tokens(base); rc = sqfs_count_tokens(rel); if (bc < 1 || rc < 1) return NULL;
- base_tokens = malloc(bc * sizeof(char *)); + base_tokens = calloc(bc, sizeof(char *)); if (!base_tokens) return NULL;
- rel_tokens = malloc(rc * sizeof(char *)); + rel_tokens = calloc(rc, sizeof(char *)); if (!rel_tokens) - goto free_b_tokens; + goto out;
/* Fill token lists */ ret = sqfs_tokenize(base_tokens, bc, base); if (ret) - goto free_r_tokens; + goto out;
ret = sqfs_tokenize(rel_tokens, rc, rel); if (ret) - goto free_r_tokens; + goto out;
/* count '..' occurrences in target path */ for (i = 0; i < rc; i++) { @@ -372,7 +375,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) /* Remove the last token and the '..' occurrences */ bc = sqfs_clean_base_path(base_tokens, bc, updir); if (bc < 0) - goto free_r_tokens; + goto out;
/* Calculate resolved path size */ if (!bc) @@ -383,7 +386,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel)
resolved = malloc(resolved_size + 1); if (!resolved) - goto free_r_tokens_loop; + goto out;
/* Set resolved path */ memset(resolved, '\0', resolved_size + 1); @@ -391,14 +394,15 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) resolved[offset++] = '/'; offset += sqfs_join(rel_tokens, resolved + offset, updir, rc, '/');
-free_r_tokens_loop: - for (i = 0; i < rc; i++) - free(rel_tokens[i]); - for (i = 0; i < bc; i++) - free(base_tokens[i]); -free_r_tokens: +out: + if (rel_tokens) + for (i = 0; i < rc; i++) + free(rel_tokens[i]); + if (base_tokens) + for (i = 0; i < bc; i++) + free(base_tokens[i]); + free(rel_tokens); -free_b_tokens: free(base_tokens);
return resolved;
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
if sqfs_tokenize(rel_tokens, rc, rel); fails, the function exits without freeing the array base_tokens.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 825d5d13fa2..f41deece0ae 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -340,28 +340,31 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) char **base_tokens, **rel_tokens, *resolved = NULL; int ret, bc, rc, i, updir = 0, resolved_size = 0, offset = 0;
base_tokens = NULL;rel_tokens = NULL;/* Memory allocation for the token lists */ bc = sqfs_count_tokens(base); rc = sqfs_count_tokens(rel); if (bc < 1 || rc < 1) return NULL;
base_tokens = malloc(bc * sizeof(char *));
base_tokens = calloc(bc, sizeof(char *)); if (!base_tokens) return NULL;
rel_tokens = malloc(rc * sizeof(char *));
rel_tokens = calloc(rc, sizeof(char *)); if (!rel_tokens)
goto free_b_tokens;
goto out; /* Fill token lists */ ret = sqfs_tokenize(base_tokens, bc, base); if (ret)
goto free_r_tokens;
goto out; ret = sqfs_tokenize(rel_tokens, rc, rel); if (ret)
goto free_r_tokens;
goto out; /* count '..' occurrences in target path */ for (i = 0; i < rc; i++) {@@ -372,7 +375,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) /* Remove the last token and the '..' occurrences */ bc = sqfs_clean_base_path(base_tokens, bc, updir); if (bc < 0)
goto free_r_tokens;
goto out; /* Calculate resolved path size */ if (!bc)@@ -383,7 +386,7 @@ static char *sqfs_get_abs_path(const char *base, const char *rel)
resolved = malloc(resolved_size + 1); if (!resolved)
goto free_r_tokens_loop;
goto out; /* Set resolved path */ memset(resolved, '\0', resolved_size + 1);@@ -391,14 +394,15 @@ static char *sqfs_get_abs_path(const char *base, const char *rel) resolved[offset++] = '/'; offset += sqfs_join(rel_tokens, resolved + offset, updir, rc, '/');
-free_r_tokens_loop:
for (i = 0; i < rc; i++)free(rel_tokens[i]);for (i = 0; i < bc; i++)free(base_tokens[i]);-free_r_tokens: +out:
if (rel_tokens)for (i = 0; i < rc; i++)free(rel_tokens[i]);if (base_tokens)for (i = 0; i < bc; i++)free(base_tokens[i]);free(rel_tokens);-free_b_tokens: free(base_tokens);
return resolved;
On Tue, Nov 03, 2020 at 12:11:17PM +0100, Richard Genoud wrote:
if sqfs_tokenize(rel_tokens, rc, rel); fails, the function exits without freeing the array base_tokens.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
finfo.blk_sizes may not be freed in case of error in the for loop Setting it to null and freeing it at the end makes prevents that from happening.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 48 +++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index f41deece0ae..d8d4584fbfd 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1305,8 +1305,8 @@ static int sqfs_get_lregfile_info(struct squashfs_lreg_inode *lreg, int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actread) { - char *dir, *fragment_block, *datablock = NULL, *data_buffer = NULL; - char *fragment, *file, *resolved, *data; + char *dir = NULL, *fragment_block, *datablock = NULL, *data_buffer = NULL; + char *fragment = NULL, *file = NULL, *resolved, *data; u64 start, n_blks, table_size, data_offset, table_offset; int ret, j, i_number, datablk_count = 0; struct squashfs_super_block *sblk = ctxt.sblk; @@ -1331,7 +1331,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sqfs_split_path(&file, &dir, filename); ret = sqfs_opendir(dir, &dirsp); if (ret) { - goto free_paths; + goto out; }
dirs = (struct squashfs_dir_stream *)dirsp; @@ -1350,7 +1350,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, printf("File not found.\n"); *actread = 0; ret = -ENOENT; - goto free_paths; + goto out; }
i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset; @@ -1365,7 +1365,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sblk->block_size); if (datablk_count < 0) { ret = -EINVAL; - goto free_paths; + goto out; }
memcpy(finfo.blk_sizes, ipos + sizeof(*reg), @@ -1378,7 +1378,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sblk->block_size); if (datablk_count < 0) { ret = -EINVAL; - goto free_paths; + goto out; }
memcpy(finfo.blk_sizes, ipos + sizeof(*lreg), @@ -1390,7 +1390,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, resolved = sqfs_resolve_symlink(symlink, filename); ret = sqfs_read(resolved, buf, offset, len, actread); free(resolved); - goto free_paths; + goto out; case SQFS_BLKDEV_TYPE: case SQFS_CHRDEV_TYPE: case SQFS_LBLKDEV_TYPE: @@ -1402,14 +1402,14 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, default: printf("Unsupported entry type\n"); ret = -EINVAL; - goto free_paths; + goto out; }
/* If the user specifies a length, check its sanity */ if (len) { if (len > finfo.size) { ret = -EINVAL; - goto free_paths; + goto out; }
finfo.size = len; @@ -1420,7 +1420,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, datablock = malloc(get_unaligned_le32(&sblk->block_size)); if (!datablock) { ret = -ENOMEM; - goto free_paths; + goto out; } }
@@ -1435,7 +1435,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
if (!data_buffer) { ret = -ENOMEM; - goto free_datablk; + goto out; }
ret = sqfs_disk_read(start, n_blks, data_buffer); @@ -1446,7 +1446,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, * image with mksquashfs's -b <block_size> option. */ printf("Error: too many data blocks to be read.\n"); - goto free_buffer; + goto out; }
data = data_buffer + table_offset; @@ -1457,7 +1457,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, ret = sqfs_decompress(&ctxt, datablock, &dest_len, data, table_size); if (ret) - goto free_buffer; + goto out;
memcpy(buf + offset + *actread, datablock, dest_len); *actread += dest_len; @@ -1471,14 +1471,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, data_buffer = NULL; }
- free(finfo.blk_sizes); - /* * There is no need to continue if the file is not fragmented. */ if (!finfo.frag) { ret = 0; - goto free_buffer; + goto out; }
start = frag_entry.start / ctxt.cur_dev->blksz; @@ -1490,12 +1488,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
if (!fragment) { ret = -ENOMEM; - goto free_buffer; + goto out; }
ret = sqfs_disk_read(start, n_blks, fragment); if (ret < 0) - goto free_fragment; + goto out;
/* File compressed and fragmented */ if (finfo.frag && finfo.comp) { @@ -1503,7 +1501,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, fragment_block = malloc(dest_len); if (!fragment_block) { ret = -ENOMEM; - goto free_fragment; + goto out; }
ret = sqfs_decompress(&ctxt, fragment_block, &dest_len, @@ -1511,7 +1509,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, frag_entry.size); if (ret) { free(fragment_block); - goto free_fragment; + goto out; }
for (j = offset + *actread; j < finfo.size; j++) { @@ -1530,17 +1528,15 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, } }
-free_fragment: +out: free(fragment); -free_buffer: - if (datablk_count) + if (datablk_count) { free(data_buffer); -free_datablk: - if (datablk_count) free(datablock); -free_paths: + } free(file); free(dir); + free(finfo.blk_sizes); sqfs_closedir(dirsp);
return ret;
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
finfo.blk_sizes may not be freed in case of error in the for loop Setting it to null and freeing it at the end makes prevents that from happening.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 48 +++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index f41deece0ae..d8d4584fbfd 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1305,8 +1305,8 @@ static int sqfs_get_lregfile_info(struct squashfs_lreg_inode *lreg, int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actread) {
char *dir, *fragment_block, *datablock = NULL, *data_buffer = NULL;char *fragment, *file, *resolved, *data;
char *dir = NULL, *fragment_block, *datablock = NULL, *data_buffer= NULL;
char *fragment = NULL, *file = NULL, *resolved, *data; u64 start, n_blks, table_size, data_offset, table_offset; int ret, j, i_number, datablk_count = 0; struct squashfs_super_block *sblk = ctxt.sblk;@@ -1331,7 +1331,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sqfs_split_path(&file, &dir, filename); ret = sqfs_opendir(dir, &dirsp); if (ret) {
goto free_paths;
goto out; } dirs = (struct squashfs_dir_stream *)dirsp;@@ -1350,7 +1350,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, printf("File not found.\n"); *actread = 0; ret = -ENOENT;
goto free_paths;
goto out; } i_number = dirs->dir_header->inode_number +dirs->entry->inode_offset; @@ -1365,7 +1365,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sblk->block_size); if (datablk_count < 0) { ret = -EINVAL;
goto free_paths;
goto out; } memcpy(finfo.blk_sizes, ipos + sizeof(*reg),@@ -1378,7 +1378,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, sblk->block_size); if (datablk_count < 0) { ret = -EINVAL;
goto free_paths;
goto out; } memcpy(finfo.blk_sizes, ipos + sizeof(*lreg),@@ -1390,7 +1390,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, resolved = sqfs_resolve_symlink(symlink, filename); ret = sqfs_read(resolved, buf, offset, len, actread); free(resolved);
goto free_paths;
goto out; case SQFS_BLKDEV_TYPE: case SQFS_CHRDEV_TYPE: case SQFS_LBLKDEV_TYPE:@@ -1402,14 +1402,14 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, default: printf("Unsupported entry type\n"); ret = -EINVAL;
goto free_paths;
goto out; } /* If the user specifies a length, check its sanity */ if (len) { if (len > finfo.size) { ret = -EINVAL;
goto free_paths;
goto out; } finfo.size = len;@@ -1420,7 +1420,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, datablock = malloc(get_unaligned_le32(&sblk->block_size)); if (!datablock) { ret = -ENOMEM;
goto free_paths;
goto out; } }@@ -1435,7 +1435,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
if (!data_buffer) { ret = -ENOMEM;
goto free_datablk;
goto out; } ret = sqfs_disk_read(start, n_blks, data_buffer);@@ -1446,7 +1446,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, * image with mksquashfs's -b <block_size> option. */ printf("Error: too many data blocks to be read.\n");
goto free_buffer;
goto out; } data = data_buffer + table_offset;@@ -1457,7 +1457,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, ret = sqfs_decompress(&ctxt, datablock, &dest_len, data, table_size); if (ret)
goto free_buffer;
goto out; memcpy(buf + offset + *actread, datablock,dest_len); *actread += dest_len; @@ -1471,14 +1471,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, data_buffer = NULL; }
free(finfo.blk_sizes);/* * There is no need to continue if the file is not fragmented. */ if (!finfo.frag) { ret = 0;goto free_buffer;
goto out; } start = frag_entry.start / ctxt.cur_dev->blksz;@@ -1490,12 +1488,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
if (!fragment) { ret = -ENOMEM;
goto free_buffer;
goto out; } ret = sqfs_disk_read(start, n_blks, fragment); if (ret < 0)
goto free_fragment;
goto out; /* File compressed and fragmented */ if (finfo.frag && finfo.comp) {@@ -1503,7 +1501,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, fragment_block = malloc(dest_len); if (!fragment_block) { ret = -ENOMEM;
goto free_fragment;
goto out; } ret = sqfs_decompress(&ctxt, fragment_block, &dest_len,@@ -1511,7 +1509,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, frag_entry.size); if (ret) { free(fragment_block);
goto free_fragment;
goto out; } for (j = offset + *actread; j < finfo.size; j++) {@@ -1530,17 +1528,15 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, } }
-free_fragment: +out: free(fragment); -free_buffer:
if (datablk_count)
if (datablk_count) { free(data_buffer);-free_datablk:
if (datablk_count) free(datablock);-free_paths:
} free(file); free(dir);free(finfo.blk_sizes); sqfs_closedir(dirsp); return ret;
On Tue, Nov 03, 2020 at 12:11:18PM +0100, Richard Genoud wrote:
finfo.blk_sizes may not be freed in case of error in the for loop Setting it to null and freeing it at the end makes prevents that from happening.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
If SquashFS magic number is invalid, there's a memory leak.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index d8d4584fbfd..7d6f0e88e31 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1090,8 +1090,8 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition /* Make sure it has a valid SquashFS magic number*/ if (get_unaligned_le32(&sblk->s_magic) != SQFS_MAGIC_NUMBER) { printf("Bad magic number for SquashFS image.\n"); - ctxt.cur_dev = NULL; - return -EINVAL; + ret = -EINVAL; + goto error; }
ctxt.sblk = sblk; @@ -1099,12 +1099,16 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition ret = sqfs_decompressor_init(&ctxt);
if (ret) { - ctxt.cur_dev = NULL; - free(ctxt.sblk); - return -EINVAL; + ret = -EINVAL; + goto error; }
return 0; +error: + ctxt.cur_dev = NULL; + free(ctxt.sblk); + ctxt.sblk = NULL; + return ret; }
static char *sqfs_basename(char *path)
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
If SquashFS magic number is invalid, there's a memory leak.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index d8d4584fbfd..7d6f0e88e31 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1090,8 +1090,8 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition /* Make sure it has a valid SquashFS magic number*/ if (get_unaligned_le32(&sblk->s_magic) != SQFS_MAGIC_NUMBER) { printf("Bad magic number for SquashFS image.\n");
ctxt.cur_dev = NULL;return -EINVAL;
ret = -EINVAL;goto error; } ctxt.sblk = sblk;@@ -1099,12 +1099,16 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition ret = sqfs_decompressor_init(&ctxt);
if (ret) {
ctxt.cur_dev = NULL;free(ctxt.sblk);return -EINVAL;
ret = -EINVAL;goto error; } return 0;+error:
ctxt.cur_dev = NULL;free(ctxt.sblk);ctxt.sblk = NULL;return ret;}
static char *sqfs_basename(char *path)
On Tue, Nov 03, 2020 at 12:11:19PM +0100, Richard Genoud wrote:
If SquashFS magic number is invalid, there's a memory leak.
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
This will prevent a double free error if sqfs_close() is called twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 7d6f0e88e31..a46e19c75c5 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -49,6 +49,7 @@ static int sqfs_read_sblk(struct squashfs_super_block **sblk)
if (sqfs_disk_read(0, 1, *sblk) != 1) { free(*sblk); + sblk = NULL; return -EINVAL; }
@@ -1638,6 +1639,7 @@ free_strings: void sqfs_close(void) { free(ctxt.sblk); + ctxt.sblk = NULL; ctxt.cur_dev = NULL; sqfs_decompressor_cleanup(&ctxt); }
On Tue, Nov 03, 2020 at 12:11:20PM +0100, Richard Genoud wrote:
This will prevent a double free error if sqfs_close() is called twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
This change causes the test.py squashfs tests to fail. I am unsure if the problem is with the tests or this exposing further problems in the code.
Le 20/11/2020 à 02:35, Tom Rini a écrit :
On Tue, Nov 03, 2020 at 12:11:20PM +0100, Richard Genoud wrote:
This will prevent a double free error if sqfs_close() is called twice.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
This change causes the test.py squashfs tests to fail. I am unsure if the problem is with the tests or this exposing further problems in the code.
I'll look into that.
Thanks !
Resetting the context on error will prevent some checks like: if (!ctx.cur_dev) To pass when the probe method has failed
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index a46e19c75c5..9682ea52557 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1086,7 +1086,7 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition
ret = sqfs_read_sblk(&sblk); if (ret) - return ret; + goto error;
/* Make sure it has a valid SquashFS magic number*/ if (get_unaligned_le32(&sblk->s_magic) != SQFS_MAGIC_NUMBER) {
On Tue, Nov 03, 2020 at 12:11:21PM +0100, Richard Genoud wrote:
Resetting the context on error will prevent some checks like: if (!ctx.cur_dev) To pass when the probe method has failed
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
sqfs_decompressor_init() returns a value, so it's better to use it than to force the return value to EINVAL (it could be ENOMEM)
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 9682ea52557..fa3120aefd6 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1098,9 +1098,7 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition ctxt.sblk = sblk;
ret = sqfs_decompressor_init(&ctxt); - if (ret) { - ret = -EINVAL; goto error; }
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
sqfs_decompressor_init() returns a value, so it's better to use it than to force the return value to EINVAL (it could be ENOMEM)
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/squashfs/sqfs.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 9682ea52557..fa3120aefd6 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1098,9 +1098,7 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, struct disk_partition *fs_partition ctxt.sblk = sblk;
ret = sqfs_decompressor_init(&ctxt);
if (ret) {ret = -EINVAL; goto error; }
On Tue, Nov 03, 2020 at 12:11:22PM +0100, Richard Genoud wrote:
sqfs_decompressor_init() returns a value, so it's better to use it than to force the return value to EINVAL (it could be ENOMEM)
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
The length of the buffer wasn't taken into account when writing to the given buffer.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index fa3120aefd6..4710b2bd707 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1416,6 +1416,8 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, }
finfo.size = len; + } else { + len = finfo.size; }
if (datablk_count) { @@ -1462,9 +1464,13 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, if (ret) goto out;
+ if ((*actread + dest_len) > len) + dest_len = len - *actread; memcpy(buf + offset + *actread, datablock, dest_len); *actread += dest_len; } else { + if ((*actread + table_size) > len) + table_size = len - *actread; memcpy(buf + offset + *actread, data, table_size); *actread += table_size; } @@ -1472,6 +1478,8 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, data_offset += table_size; free(data_buffer); data_buffer = NULL; + if (*actread >= len) + break; }
/*
On Tue, Nov 03, 2020 at 12:11:23PM +0100, Richard Genoud wrote:
The length of the buffer wasn't taken into account when writing to the given buffer.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
offset is the offset in the file read, not the offset in the destination buffer. If the offset is not null, this will lead to a memory corruption. So, for now, we are returning an error if the offset is used.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 4710b2bd707..f63a06fd40f 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1327,6 +1327,14 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
*actread = 0;
+ if (offset) { + /* + * TODO: implement reading at an offset in file + */ + printf("Error: reading at a specific offset in a squashfs file is not supported yet.\n"); + return -EINVAL; + } + /* * sqfs_opendir will uncompress inode and directory tables, and will * return a pointer to the directory that contains the requested file. @@ -1466,12 +1474,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
if ((*actread + dest_len) > len) dest_len = len - *actread; - memcpy(buf + offset + *actread, datablock, dest_len); + memcpy(buf + *actread, datablock, dest_len); *actread += dest_len; } else { if ((*actread + table_size) > len) table_size = len - *actread; - memcpy(buf + offset + *actread, data, table_size); + memcpy(buf + *actread, data, table_size); *actread += table_size; }
@@ -1523,7 +1531,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, goto out; }
- for (j = offset + *actread; j < finfo.size; j++) { + for (j = *actread; j < finfo.size; j++) { memcpy(buf + j, &fragment_block[finfo.offset + j], 1); (*actread)++; } @@ -1533,7 +1541,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, } else if (finfo.frag && !finfo.comp) { fragment_block = (void *)fragment + table_offset;
- for (j = offset + *actread; j < finfo.size; j++) { + for (j = *actread; j < finfo.size; j++) { memcpy(buf + j, &fragment_block[finfo.offset + j], 1); (*actread)++; }
On Tue, Nov 03, 2020 at 12:11:24PM +0100, Richard Genoud wrote:
offset is the offset in the file read, not the offset in the destination buffer. If the offset is not null, this will lead to a memory corruption. So, for now, we are returning an error if the offset is used.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
The code for reading a fragmented file is not functionnal. It's better to signal this to the user.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/squashfs/sqfs.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index f63a06fd40f..a96c1d4f564 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1498,6 +1498,13 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, goto out; }
+ printf("Error: reading a fragmented file is not supported yet.\n"); + ret = -EINVAL; + goto out; + + /* + * TODO: reading a fragmented file doesn't work + */ start = frag_entry.start / ctxt.cur_dev->blksz; table_size = SQFS_BLOCK_SIZE(frag_entry.size); table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz);
On Tue, Nov 03, 2020 at 12:11:25PM +0100, Richard Genoud wrote:
The code for reading a fragmented file is not functionnal. It's better to signal this to the user.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
This change causes the test.py squashfs tests to fail. I am unsure if the problem is with the tests or this exposing further problems in the code.
Hi,
Le 20/11/2020 à 02:35, Tom Rini a écrit :
On Tue, Nov 03, 2020 at 12:11:25PM +0100, Richard Genoud wrote:
The code for reading a fragmented file is not functionnal. It's better to signal this to the user.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
This change causes the test.py squashfs tests to fail. I am unsure if the problem is with the tests or this exposing further problems in the code.
Actually, reading a fragmented file doesn't work. The test only check if the file is read, but not it's content.
With this following patch, we'll see that the file content is not the same :
From 68f87301c059aaae8e90e42fbec9b560aee0c6eb Mon Sep 17 00:00:00 2001 From: Richard Genoud richard.genoud@posteo.net Date: Tue, 24 Nov 2020 17:45:07 +0100 Subject: [PATCH] test/py: SquashFS: Check if loaded file is corrupted
After loading the file in memory, its content should be checked for errors.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- test/py/tests/test_fs/test_squashfs/sqfs_common.py | 5 ++++- test/py/tests/test_fs/test_squashfs/test_sqfs_load.py | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py index c96f92c1d8f..a7673c73762 100644 --- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py +++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py @@ -6,6 +6,7 @@ import os import random import string import subprocess +import zlib
def sqfs_get_random_letters(size): letters = [] @@ -19,12 +20,14 @@ def sqfs_generate_file(path, size): file = open(path, "w") file.write(content) file.close() + return zlib.crc32(content.encode())
class Compression: def __init__(self, name, files, sizes, block_size = 4096): self.name = name self.files = files self.sizes = sizes + self.crc = [] self.mksquashfs_opts = " -b " + str(block_size) + " -comp " + self.name
def add_opt(self, opt): @@ -34,7 +37,7 @@ class Compression: src = os.path.join(build_dir, "sqfs_src/") os.mkdir(src) for (f, s) in zip(self.files, self.sizes): - sqfs_generate_file(src + f, s) + self.crc.append(sqfs_generate_file(src + f, s))
# the symbolic link always targets the first file os.symlink(self.files[0], src + "sym") diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py index 9e900623846..2ab4660036e 100644 --- a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py +++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py @@ -4,6 +4,7 @@
import os import pytest +import zlib from sqfs_common import *
@pytest.mark.boardspec('sandbox') @@ -14,6 +15,7 @@ from sqfs_common import * def test_sqfs_load(u_boot_console): build_dir = u_boot_console.config.build_dir command = "sqfsload host 0 $kernel_addr_r " + sum_command = "crc32 -v $kernel_addr_r $filesize "
for opt in comp_opts: # generate and load the squashfs image @@ -30,10 +32,12 @@ def test_sqfs_load(u_boot_console): output = u_boot_console.run_command(command + "xxx") assert "File not found." in output
- for (f, s) in zip(opt.files, opt.sizes): + for (f, s, c) in zip(opt.files, opt.sizes, opt.crc): try: output = u_boot_console.run_command(command + f) assert str(s) in output + output = u_boot_console.run_command(sum_command + format(c, '08x')) + assert not 'ERROR' in output except: assert False opt.cleanup(build_dir)
Hi Joao,
On Wed, 25 Nov 2020 at 01:58, Richard Genoud richard.genoud@posteo.net wrote:
Hi,
Le 20/11/2020 à 02:35, Tom Rini a écrit :
On Tue, Nov 03, 2020 at 12:11:25PM +0100, Richard Genoud wrote:
The code for reading a fragmented file is not functionnal. It's better to signal this to the user.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
This change causes the test.py squashfs tests to fail. I am unsure if the problem is with the tests or this exposing further problems in the code.
Actually, reading a fragmented file doesn't work. The test only check if the file is read, but not it's content.
With this following patch, we'll see that the file content is not the same :
From 68f87301c059aaae8e90e42fbec9b560aee0c6eb Mon Sep 17 00:00:00 2001 From: Richard Genoud richard.genoud@posteo.net Date: Tue, 24 Nov 2020 17:45:07 +0100 Subject: [PATCH] test/py: SquashFS: Check if loaded file is corrupted
After loading the file in memory, its content should be checked for errors.
Signed-off-by: Richard Genoud richard.genoud@posteo.net
test/py/tests/test_fs/test_squashfs/sqfs_common.py | 5 ++++- test/py/tests/test_fs/test_squashfs/test_sqfs_load.py | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py index c96f92c1d8f..a7673c73762 100644 --- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py +++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Thanks, Simon
Em qua., 27 de jan. de 2021 às 12:15, Simon Glass sjg@chromium.org escreveu:
Hi Joao,
Hello!
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Absolutely. Excuse me for such a late reply.
Thanks, Simon
Hi,
On Thu, 4 Feb 2021 at 15:32, João Marcos Costa jmcosta944@gmail.com wrote:
Em qua., 27 de jan. de 2021 às 12:15, Simon Glass sjg@chromium.org escreveu:
Hi Joao,
Hello!
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Absolutely. Excuse me for such a late reply.
Any word on this please? Have you been able to repeat this?
Regards, Simon
Hi all,
Le 08/05/2021 à 23:51, Simon Glass a écrit :
Hi,
On Thu, 4 Feb 2021 at 15:32, João Marcos Costa jmcosta944@gmail.com wrote:
Em qua., 27 de jan. de 2021 às 12:15, Simon Glass sjg@chromium.org escreveu:
Hi Joao,
Hello!
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Absolutely. Excuse me for such a late reply.
Any word on this please? Have you been able to repeat this?
Yes, for me, reading fragmented files doesn't work. The test "test_sqfs_load" is OK because it only tests the file length not its content.
I've written a patch to check if the file is corrupted or not, and it fails : ./test/py/test.py --bd sandbox --build -k test_sqfs_load -v [...] AssertionError: assert not 'ERROR' in 'crc32 for 01000000 ... 010013eb ==> df8e6fe2 != d1522690 ** ERROR **' [...]
Here's the patch onto v2021.07-rc2 : ----------8<-------- Subject: [PATCH] test/py: SquashFS: Check if loaded file is corrupted
After loading the file in memory, its content should be checked for errors.
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- test/py/tests/test_fs/test_squashfs/sqfs_common.py | 5 ++++- test/py/tests/test_fs/test_squashfs/test_sqfs_load.py | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_fs/test_squashfs/sqfs_common.py b/test/py/tests/test_fs/test_squashfs/sqfs_common.py index c96f92c1d8f..a7673c73762 100644 --- a/test/py/tests/test_fs/test_squashfs/sqfs_common.py +++ b/test/py/tests/test_fs/test_squashfs/sqfs_common.py @@ -6,6 +6,7 @@ import os import random import string import subprocess +import zlib
def sqfs_get_random_letters(size): letters = [] @@ -19,12 +20,14 @@ def sqfs_generate_file(path, size): file = open(path, "w") file.write(content) file.close() + return zlib.crc32(content.encode())
class Compression: def __init__(self, name, files, sizes, block_size = 4096): self.name = name self.files = files self.sizes = sizes + self.crc = [] self.mksquashfs_opts = " -b " + str(block_size) + " -comp " + self.name
def add_opt(self, opt): @@ -34,7 +37,7 @@ class Compression: src = os.path.join(build_dir, "sqfs_src/") os.mkdir(src) for (f, s) in zip(self.files, self.sizes): - sqfs_generate_file(src + f, s) + self.crc.append(sqfs_generate_file(src + f, s))
# the symbolic link always targets the first file os.symlink(self.files[0], src + "sym") diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py index 9e900623846..2ab4660036e 100644 --- a/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py +++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_load.py @@ -4,6 +4,7 @@
import os import pytest +import zlib from sqfs_common import *
@pytest.mark.boardspec('sandbox') @@ -14,6 +15,7 @@ from sqfs_common import * def test_sqfs_load(u_boot_console): build_dir = u_boot_console.config.build_dir command = "sqfsload host 0 $kernel_addr_r " + sum_command = "crc32 -v $kernel_addr_r $filesize "
for opt in comp_opts: # generate and load the squashfs image @@ -30,10 +32,12 @@ def test_sqfs_load(u_boot_console): output = u_boot_console.run_command(command + "xxx") assert "File not found." in output
- for (f, s) in zip(opt.files, opt.sizes): + for (f, s, c) in zip(opt.files, opt.sizes, opt.crc): try: output = u_boot_console.run_command(command + f) assert str(s) in output + output = u_boot_console.run_command(sum_command + format(c, '08x')) + assert not 'ERROR' in output except: assert False opt.cleanup(build_dir)
Hello,
Em ter., 11 de mai. de 2021 às 10:04, Richard Genoud < richard.genoud@posteo.net> escreveu:
Hi all,
Le 08/05/2021 à 23:51, Simon Glass a écrit :
Hi,
On Thu, 4 Feb 2021 at 15:32, João Marcos Costa jmcosta944@gmail.com
wrote:
Em qua., 27 de jan. de 2021 às 12:15, Simon Glass sjg@chromium.org
escreveu:
Hi Joao,
Hello!
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Absolutely. Excuse me for such a late reply.
Any word on this please? Have you been able to repeat this?
Yes, for me, reading fragmented files doesn't work. The test "test_sqfs_load" is OK because it only tests the file length not its content.
I've written a patch to check if the file is corrupted or not, and it fails : ./test/py/test.py --bd sandbox --build -k test_sqfs_load -v [...]
I finally could get back to SquashFS support today, and I fixed a few bugs concerning the fragmented files. However, I still need to run a few more tests before submitting the patches.
Best regards, Joao Marcos
www.linkedin.com/in/jmarcoscosta/ https://github.com/jmarcoscosta
On Sun, May 16, 2021 at 09:44:53PM -0300, João Marcos Costa wrote:
Hello,
Em ter., 11 de mai. de 2021 às 10:04, Richard Genoud < richard.genoud@posteo.net> escreveu:
Hi all,
Le 08/05/2021 à 23:51, Simon Glass a écrit :
Hi,
On Thu, 4 Feb 2021 at 15:32, João Marcos Costa jmcosta944@gmail.com
wrote:
Em qua., 27 de jan. de 2021 às 12:15, Simon Glass sjg@chromium.org
escreveu:
Hi Joao,
Hello!
This test works the first time I run it but fails the second time, since the directory already exists. This makes it necessary to disable the test for development.
It also uses the wrong quoting style - we have settled on a single quote by default in U-Boot.
Finally, the tests and some functions need comments about what they do and what the arguments are.
Please can you take a look?
Absolutely. Excuse me for such a late reply.
Any word on this please? Have you been able to repeat this?
Yes, for me, reading fragmented files doesn't work. The test "test_sqfs_load" is OK because it only tests the file length not its content.
I've written a patch to check if the file is corrupted or not, and it fails : ./test/py/test.py --bd sandbox --build -k test_sqfs_load -v [...]
I finally could get back to SquashFS support today, and I fixed a few bugs concerning the fragmented files. However, I still need to run a few more tests before submitting the patches.
Please make sure the problem with repeatedly running the tests is fixed as well, that's what's blocking using the public gitlab CI runners, thanks!
This permits to find a file and use the distro_bootcmd
Signed-off-by: Richard Genoud richard.genoud@posteo.net --- fs/fs.c | 2 +- fs/squashfs/sqfs.c | 38 ++++++++++++++++++++++++++++++++++++++ include/squashfs.h | 1 + 3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/fs/fs.c b/fs/fs.c index fb27c910d4f..7a4020607a3 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -296,7 +296,7 @@ static struct fstype_info fstypes[] = { .size = sqfs_size, .close = sqfs_close, .closedir = sqfs_closedir, - .exists = fs_exists_unsupported, + .exists = sqfs_exists, .uuid = fs_uuid_unsupported, .write = fs_write_unsupported, .ln = fs_ln_unsupported, diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index a96c1d4f564..c8e8d682cd4 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1657,6 +1657,44 @@ free_strings: return ret; }
+int sqfs_exists(const char *filename) +{ + struct fs_dir_stream *dirsp = NULL; + struct squashfs_dir_stream *dirs; + char *dir, *file; + struct fs_dirent *dent; + int ret; + + sqfs_split_path(&file, &dir, filename); + /* + * sqfs_opendir will uncompress inode and directory tables, and will + * return a pointer to the directory that contains the requested file. + */ + ret = sqfs_opendir(dir, &dirsp); + if (ret) { + ret = -EINVAL; + goto free_strings; + } + + dirs = (struct squashfs_dir_stream *)dirsp; + + while (!sqfs_readdir(dirsp, &dent)) { + ret = strcmp(dent->name, file); + if (!ret) + break; + free(dirs->entry); + dirs->entry = NULL; + } + + sqfs_closedir(dirsp); + +free_strings: + free(dir); + free(file); + + return ret == 0; +} + void sqfs_close(void) { free(ctxt.sblk); diff --git a/include/squashfs.h b/include/squashfs.h index 819cf8c2da8..7489eefa1f2 100644 --- a/include/squashfs.h +++ b/include/squashfs.h @@ -19,6 +19,7 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actread); int sqfs_size(const char *filename, loff_t *size); +int sqfs_exists(const char *filename); void sqfs_close(void); void sqfs_closedir(struct fs_dir_stream *dirs);
Reviewed-by Joao Marcos Costa jmcosta944@gmail.com
Em ter., 3 de nov. de 2020 às 08:12, Richard Genoud < richard.genoud@posteo.net> escreveu:
This permits to find a file and use the distro_bootcmd
Signed-off-by: Richard Genoud richard.genoud@posteo.net
fs/fs.c | 2 +- fs/squashfs/sqfs.c | 38 ++++++++++++++++++++++++++++++++++++++ include/squashfs.h | 1 + 3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/fs/fs.c b/fs/fs.c index fb27c910d4f..7a4020607a3 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -296,7 +296,7 @@ static struct fstype_info fstypes[] = { .size = sqfs_size, .close = sqfs_close, .closedir = sqfs_closedir,
.exists = fs_exists_unsupported,
.exists = sqfs_exists, .uuid = fs_uuid_unsupported, .write = fs_write_unsupported, .ln = fs_ln_unsupported,diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index a96c1d4f564..c8e8d682cd4 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -1657,6 +1657,44 @@ free_strings: return ret; }
+int sqfs_exists(const char *filename) +{
struct fs_dir_stream *dirsp = NULL;struct squashfs_dir_stream *dirs;char *dir, *file;struct fs_dirent *dent;int ret;sqfs_split_path(&file, &dir, filename);/** sqfs_opendir will uncompress inode and directory tables, andwill
* return a pointer to the directory that contains the requestedfile.
*/ret = sqfs_opendir(dir, &dirsp);if (ret) {ret = -EINVAL;goto free_strings;}dirs = (struct squashfs_dir_stream *)dirsp;while (!sqfs_readdir(dirsp, &dent)) {ret = strcmp(dent->name, file);if (!ret)break;free(dirs->entry);dirs->entry = NULL;}sqfs_closedir(dirsp);+free_strings:
free(dir);free(file);return ret == 0;+}
void sqfs_close(void) { free(ctxt.sblk); diff --git a/include/squashfs.h b/include/squashfs.h index 819cf8c2da8..7489eefa1f2 100644 --- a/include/squashfs.h +++ b/include/squashfs.h @@ -19,6 +19,7 @@ int sqfs_probe(struct blk_desc *fs_dev_desc, int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actread); int sqfs_size(const char *filename, loff_t *size); +int sqfs_exists(const char *filename); void sqfs_close(void); void sqfs_closedir(struct fs_dir_stream *dirs);
On Tue, Nov 03, 2020 at 12:11:26PM +0100, Richard Genoud wrote:
This permits to find a file and use the distro_bootcmd
Reviewed-by: Joao Marcos Costa jmcosta944@gmail.com Signed-off-by: Richard Genoud richard.genoud@posteo.net
Applied to u-boot/master, thanks!
Le 03/11/2020 à 12:10, Richard Genoud a écrit :
This patch series fix several memory leaks, some use of dangling pointers (leading to cpu freeze) and finally introduce the exists() function for squashfs. This function enable testing the existence of a file, which is mandatory for using the distro_bootcmd Those fixes have been cut into several patches to be easier to review
Changes since v1:
- patch 5: *dir = *file = NULL; is split in 2 lines
Sorry, I forgot a change: - in patch 15, I removed a sqfs_closedir(dirsp) that I forgot in v1
- For consistency, sqfs_frag_lookup is modified to use a single "goto out"
( cf https://lists.denx.de/pipermail/u-boot/2020-October/429645.html )
- more memory leak fixes in sqfs_get_abs_path, sqfs_read and sqfs_probe
- a missing error check typo in sqfs_get_abs_path
- some missing reseting ctxt.sblk to NULL to prevent double free
- reset cur_dev/cur_part_info to NULL when they are freed
- return value of sqfs_decompressor_init() wasn't used
- use "len" in sqfs_read to prevent writing beyond buffer
- prevent reading with an offset since it doesn't work
- prevent reading fragmented files since it doesn't work
Richard Genoud (28): fs/squashfs: fix board hang-up when calling .exists() fs/squashfs: sqfs_opendir: fix some memory leaks and dangling pointers fs/squashfs: sqfs_opendir: simplify error handling fs/squashfs: sqfs_closedir: fix memory leak fs/squashfs: sqfs_split_path: fix memory leak and dangling pointers fs/squashfs: sqfs_read_directory_table: fix memory leak fs/squashfs: sqfs_search_dir: fix dangling pointer fs/squashfs: sqfs_search_dir: fix memory leaks fs/squashfs: sqfs_read_inode_table: fix dangling pointer fs/squashfs: sqfs_concat_tokens: check if malloc succeeds fs/squashfs: sqfs_size: fix dangling pointer dirs->entry fs/squashfs: sqfs_size: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix dangling pointer dirs->entry fs/squashfs: sqfs_read: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix memory leak fs/squashfs: sqfs_read: fix another memory leak fs/squashfs: sqfs_frag_lookup: simplify error handling fs/squashfs: sqfs_get_abs_path: fix error check fs/squashfs: sqfs_get_abs_path: fix possible memory leak on error fs/squashfs: sqfs_read: fix memory leak on finfo.blk_sizes fs/squashfs: sqfs_probe: fix possible memory leak on error fs/squashfs: sqfs_close/sqfs_read_sblk: set ctxt.sblk to NULL after free fs/squashfs: sqfs_probe: reset cur_dev/cur_part_info to NULL on error fs/squashfs: sqfs_probe: use sqfs_decompressor_init() return value fs/squashfs: sqfs_read: don't write beyond buffer size fs/squashfs: sqfs_read: remove buggy offset functionality fs/squashfs: sqfs_read: fragmented files are not supported fs/squashfs: implement exists() function
fs/fs.c | 7 + fs/squashfs/sqfs.c | 399 +++++++++++++++++++++++++++++++-------------- include/squashfs.h | 1 + 3 files changed, 286 insertions(+), 121 deletions(-)
I haven't been able to finish reviewing and testing everything yet, but I thank you in advance for those fixes!
Best regards,
Em ter., 3 de nov. de 2020 às 08:18, Richard Genoud < richard.genoud@posteo.net> escreveu:
Le 03/11/2020 à 12:10, Richard Genoud a écrit :
This patch series fix several memory leaks, some use of dangling pointers (leading to cpu freeze) and finally introduce the exists() function for squashfs. This function enable testing the existence of a file, which is mandatory for using the distro_bootcmd Those fixes have been cut into several patches to be easier to review
Changes since v1:
- patch 5: *dir = *file = NULL; is split in 2 lines
Sorry, I forgot a change:
- in patch 15, I removed a sqfs_closedir(dirsp) that I forgot in v1
- For consistency, sqfs_frag_lookup is modified to use a single "goto out"
( cf https://lists.denx.de/pipermail/u-boot/2020-October/429645.html )
- more memory leak fixes in sqfs_get_abs_path, sqfs_read and sqfs_probe
- a missing error check typo in sqfs_get_abs_path
- some missing reseting ctxt.sblk to NULL to prevent double free
- reset cur_dev/cur_part_info to NULL when they are freed
- return value of sqfs_decompressor_init() wasn't used
- use "len" in sqfs_read to prevent writing beyond buffer
- prevent reading with an offset since it doesn't work
- prevent reading fragmented files since it doesn't work
Richard Genoud (28): fs/squashfs: fix board hang-up when calling .exists() fs/squashfs: sqfs_opendir: fix some memory leaks and dangling pointers fs/squashfs: sqfs_opendir: simplify error handling fs/squashfs: sqfs_closedir: fix memory leak fs/squashfs: sqfs_split_path: fix memory leak and dangling pointers fs/squashfs: sqfs_read_directory_table: fix memory leak fs/squashfs: sqfs_search_dir: fix dangling pointer fs/squashfs: sqfs_search_dir: fix memory leaks fs/squashfs: sqfs_read_inode_table: fix dangling pointer fs/squashfs: sqfs_concat_tokens: check if malloc succeeds fs/squashfs: sqfs_size: fix dangling pointer dirs->entry fs/squashfs: sqfs_size: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix dangling pointer dirs->entry fs/squashfs: sqfs_read: remove useless sqfs_closedir() fs/squashfs: sqfs_read: fix memory leak fs/squashfs: sqfs_read: fix another memory leak fs/squashfs: sqfs_frag_lookup: simplify error handling fs/squashfs: sqfs_get_abs_path: fix error check fs/squashfs: sqfs_get_abs_path: fix possible memory leak on error fs/squashfs: sqfs_read: fix memory leak on finfo.blk_sizes fs/squashfs: sqfs_probe: fix possible memory leak on error fs/squashfs: sqfs_close/sqfs_read_sblk: set ctxt.sblk to NULL after free fs/squashfs: sqfs_probe: reset cur_dev/cur_part_info to NULL on error fs/squashfs: sqfs_probe: use sqfs_decompressor_init() return value fs/squashfs: sqfs_read: don't write beyond buffer size fs/squashfs: sqfs_read: remove buggy offset functionality fs/squashfs: sqfs_read: fragmented files are not supported fs/squashfs: implement exists() function
fs/fs.c | 7 + fs/squashfs/sqfs.c | 399 +++++++++++++++++++++++++++++++-------------- include/squashfs.h | 1 + 3 files changed, 286 insertions(+), 121 deletions(-)
participants (4)
-
João Marcos Costa -
Richard Genoud -
Simon Glass -
Tom Rini