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

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 00/17] 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 existance of a file, which is mandatory
for using the distro_bootcmd
Those fixes have been cut into several patches to be easier to review

Richard Genoud (17):
  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: implement exists() function

 fs/fs.c            |   7 ++
 fs/squashfs/sqfs.c | 238 ++++++++++++++++++++++++++++++++++-----------
 include/squashfs.h |   1 +
 3 files changed, 190 insertions(+), 56 deletions(-)

Reply | Threaded
Open this post in threaded view
|

[PATCH 01/17] 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 02/17] 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 03/17] 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 04/17] 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 05/17] 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 | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index 0ac922af9e7..55d183663a8 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,18 @@ 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 = *file = NULL;
+ }
  free(basec);
-free_dirc:
  free(dirc);
-free_tmp:
  free(tmp_path);
 
  return ret;
Reply | Threaded
Open this post in threaded view
|

[PATCH 06/17] 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 55d183663a8..c4d74fd4d6d 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 07/17] 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 c4d74fd4d6d..1df27f7b903 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 08/17] 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 1df27f7b903..eb8851a7148 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 09/17] 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 eb8851a7148..c4b7c84e9aa 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 10/17] 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 c4b7c84e9aa..24a7680aa5c 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 11/17] 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 24a7680aa5c..c5a24450e6f 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1568,6 +1568,7 @@ int sqfs_size(const char *filename, loff_t *size)
  if (!ret)
  break;
  free(dirs->entry);
+ dirs->entry = NULL;
  }
 
  if (ret) {
@@ -1581,6 +1582,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 12/17] 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 c5a24450e6f..116b5160ee3 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1556,7 +1556,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 13/17] 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 116b5160ee3..0510ae311d1 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1335,6 +1335,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 14/17] 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 0510ae311d1..79d68dddb28 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1322,7 +1322,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 15/17] 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index 79d68dddb28..6cd3ba9ce10 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1531,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 16/17] 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 6cd3ba9ce10..82bf1faf25f 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 17/17] fs/squashfs: implement exists() function

Richard Genoud-2
In reply to this post by Richard Genoud-2
This permits to find a file and use the distro_bootcmd

Signed-off-by: Richard Genoud <[hidden email]>
---
 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 82bf1faf25f..6952fc2b53e 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1627,6 +1627,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);
 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/17] fs/squashfs: sqfs_split_path: fix memory leak and dangling pointers

Miquel Raynal-2
In reply to this post by Richard Genoud-2
Hi Richard,

Thank you very much for this entire series, so far I'm fine with all
your changes, but perhaps this patch needs some editions.

Richard Genoud <[hidden email]> wrote on Wed, 14 Oct 2020
10:06:10 +0200:

> *file and *dir were not freed on error
>
> Signed-off-by: Richard Genoud <[hidden email]>
> ---
>  fs/squashfs/sqfs.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index 0ac922af9e7..55d183663a8 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;

I personally prefer the use of a different goto statement per error
condition instead of a big kfree(foo); kfree(bar); kfree(barz); even if
setting them to NULL makes it legal.

> +
>   /* 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,18 @@ 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 = *file = NULL;

I also dislike these constructions (prefer a line per assignation).

> + }
>   free(basec);
> -free_dirc:
>   free(dirc);
> -free_tmp:
>   free(tmp_path);
>  
>   return ret;

Thanks,
Miquèl
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak

Miquel Raynal-2
In reply to this post by Richard Genoud-2
Hi Richard,

Richard Genoud <[hidden email]> wrote on Wed, 14 Oct 2020
10:06:11 +0200:

> pos_list wasn't freed on every error
>
> Signed-off-by: Richard Genoud <[hidden email]>

Same comment here (and probably after as well) as in patch 05/17, not
sure this is actually relevant for the community but I prefer this:

        bar = malloc();
        ...
        if (ret)
                goto free_bar;

        foo = malloc();
        ...
        if (ret)
                goto free foo;

        ...

        foo:
                kfree(foo);
        bar:
                kfree(bar);

than:

        foo = NULL;
        bar = NULL;

        ...
        if (ret)
                goto out;
        ...
        if (ret)
                goto out;
        ...
                out:
        if (ret)
                kfree(...)



> ---
>  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 55d183663a8..c4d74fd4d6d 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;

Thanks,
Miquèl
12