[PATCH v2 00/28] fs/squashfs: fix memory leaks and introduce exists() function

classic Classic list List threaded Threaded
76 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 00/28] fs/squashfs: fix memory leaks and introduce exists() function

Richard Genoud-2
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(-)

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 01/28] fs/squashfs: fix board hang-up when calling .exists()

Richard Genoud-2
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 <[hidden email]>
---
 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
  {
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 02/28] fs/squashfs: sqfs_opendir: fix some memory leaks and dangling pointers

Richard Genoud-2
In reply to this post by Richard Genoud-2
When trying to load an non-existing file, the cpu hangs!

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 03/28] fs/squashfs: sqfs_opendir: simplify error handling

Richard Genoud-2
In reply to this post by Richard Genoud-2
Using only one label permits to prevents bugs when moving code around.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 04/28] fs/squashfs: sqfs_closedir: fix memory leak

Richard Genoud-2
In reply to this post by Richard Genoud-2
sqfs_dirs wasn't freed anywhere.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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);
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 05/28] fs/squashfs: sqfs_split_path: fix memory leak and dangling pointers

Richard Genoud-2
In reply to this post by Richard Genoud-2
*file and *dir were not freed on error

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 06/28] fs/squashfs: sqfs_read_directory_table: fix memory leak

Richard Genoud-2
In reply to this post by Richard Genoud-2
pos_list wasn't freed on every error

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 07/28] fs/squashfs: sqfs_search_dir: fix dangling pointer

Richard Genoud-2
In reply to this post by Richard Genoud-2
dirs->entry shouldn't be left dangling as it could be freed twice.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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);
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 08/28] fs/squashfs: sqfs_search_dir: fix memory leaks

Richard Genoud-2
In reply to this post by Richard Genoud-2
path, target, res, rem and sym_tokens were not free on error nor success.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
 }
 
 /*
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 09/28] fs/squashfs: sqfs_read_inode_table: fix dangling pointer

Richard Genoud-2
In reply to this post by Richard Genoud-2
inode_table should not be left dangling as it may be freed in sqfs_opendir

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
  }
 
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 10/28] fs/squashfs: sqfs_concat_tokens: check if malloc succeeds

Richard Genoud-2
In reply to this post by Richard Genoud-2
memory allocation should always be checked

Signed-off-by: Richard Genoud <[hidden email]>
---
 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++) {
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 11/28] fs/squashfs: sqfs_size: fix dangling pointer dirs->entry

Richard Genoud-2
In reply to this post by Richard Genoud-2
dirs->entry shouldn't be left dangling as it could be freed twice.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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)) {
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 12/28] fs/squashfs: sqfs_size: remove useless sqfs_closedir()

Richard Genoud-2
In reply to this post by Richard Genoud-2
as sqfs_opendir failed, there's no need to call sqfs_closedir

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
  }
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 13/28] fs/squashfs: sqfs_read: fix dangling pointer dirs->entry

Richard Genoud-2
In reply to this post by Richard Genoud-2
dirs->entry shouldn't be left dangling as it could be freed twice.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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) {
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 14/28] fs/squashfs: sqfs_read: remove useless sqfs_closedir()

Richard Genoud-2
In reply to this post by Richard Genoud-2
as sqfs_opendir failed, there's no need to call sqfs_closedir

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
  }
 
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 15/28] fs/squashfs: sqfs_read: fix memory leak

Richard Genoud-2
In reply to this post by Richard Genoud-2
sqfs_closedir() should be called to free memory allocated by
sqfs_opendir()

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 16/28] fs/squashfs: sqfs_read: fix another memory leak

Richard Genoud-2
In reply to this post by Richard Genoud-2
data_buffer was allocated in a loop and freed only once.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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);
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 17/28] fs/squashfs: sqfs_frag_lookup: simplify error handling

Richard Genoud-2
In reply to this post by Richard Genoud-2
For consistency with other functions.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 18/28] fs/squashfs: sqfs_get_abs_path: fix error check

Richard Genoud-2
In reply to this post by Richard Genoud-2
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 <[hidden email]>
---
 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;
 
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 19/28] fs/squashfs: sqfs_get_abs_path: fix possible memory leak on error

Richard Genoud-2
In reply to this post by Richard Genoud-2
if  sqfs_tokenize(rel_tokens, rc, rel); fails, the function exits
without freeing the array base_tokens.

Signed-off-by: Richard Genoud <[hidden email]>
---
 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;
1234