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

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

[Richard Genoud]

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(-)

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

[Richard Genoud]

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
 	{

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

[Richard Genoud]

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;
 }

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

[Richard Genoud]

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;
 }

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

[Richard Genoud]

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);
 }

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

[Richard Genoud]

*file and *dir were not freed on error

Signed-off-by: Richard Genoud <richard.genoud@posteo.net>
---
 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;

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

[Richard Genoud]

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 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;

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

[Richard Genoud]

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 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);

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

[Richard Genoud]

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 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;
 }
 
 /*

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

[Richard Genoud]

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 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;
 			}
 

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

[Richard Genoud]

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 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++) {

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

[Richard Genoud]

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 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)) {

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

[Richard Genoud]

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 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;
 	}

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

[Richard Genoud]

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 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) {

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

[Richard Genoud]

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 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;
 	}
 

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

[Richard Genoud]

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 | 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;
 }

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

[Richard Genoud]

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 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);

[PATCH 17/17] fs/squashfs: implement exists() function

[Richard Genoud]

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 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);
 

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

[Miquel Raynal]

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 <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020
10:06:10 +0200:

> *file and *dir were not freed on error
> 
> Signed-off-by: Richard Genoud <richard.genoud@posteo.net>
> ---
>  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

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

[Miquel Raynal]

Hi Richard,

Richard Genoud <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020
10:06:11 +0200:

> pos_list wasn't freed on every error
> 
> Signed-off-by: Richard Genoud <richard.genoud@posteo.net>

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

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

[Richard Genoud]

Hi Miquel !
Thanks for your feedback.

Le 15/10/2020 ? 15:54, Miquel Raynal a ?crit?:
> Hi Richard,
> 
> Richard Genoud <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020
> 10:06:11 +0200:
> 
>> pos_list wasn't freed on every error
>>
>> Signed-off-by: Richard Genoud <richard.genoud@posteo.net>
> 
> 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(...)

I guess it's a coding habit.
I personnaly prefer the later because I think it's less error-prone :
When moving code aroung, we don't have to move the labels and rename
the gotos.
Ex:
Let's say we have this code:
	bar = malloc();
	...
	if (ret)
		goto free_bar;

	foo = malloc();
	...
	if (ret)
		goto free_foo;
	ret = init_somthing();
	if (ret)
		goto free_foo;
	ret = dummy()
	if (ret)
		goto free_foo;

	...

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

And, we want to move, for whatever reason, init_something() and dummy()
before the foo allocation. We will have to change the code to:

	bar = malloc();
	...
	if (ret)
		goto free_bar;
	ret = init_somthing();
	if (ret)
		goto free_bar; // not free_foo anymore !
	ret = dummy()
	if (ret)
		goto free_bar; // ditto

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

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

Worse, if we have to exchange bar and foo allocation, we'll also have
to exchange the deallocation of foo and bar and change all gotos beneath :
	foo = malloc();
	...
	if (ret)
		goto free_foo;

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

	ret = init_somthing();
	if (ret)
		goto free_foo; // not free_foo anymore
	ret = dummy()
	if (ret)
		goto free_foo; //ditto


	...

// oops ! we have to exchange that !
	foo:
		kfree(foo);
	bar:
		kfree(bar);


That's why I prefer only one label and setting NULL.
If I didn't convince you, I'll change it back to multiple labels :)

> 
>> ---
>>   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
> 
Thanks !
Richard

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

[Miquel Raynal]

Hi Richard,

Richard Genoud <richard.genoud@posteo.net> wrote on Thu, 15 Oct 2020
18:29:45 +0200:

> Hi Miquel !
> Thanks for your feedback.
> 
> Le 15/10/2020 ? 15:54, Miquel Raynal a ?crit?:
> > Hi Richard,
> > 
> > Richard Genoud <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020
> > 10:06:11 +0200:
> >   
> >> pos_list wasn't freed on every error
> >>
> >> Signed-off-by: Richard Genoud <richard.genoud@posteo.net>  
> > 
> > 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(...)  
> 
> I guess it's a coding habit.
> I personnaly prefer the later because I think it's less error-prone :
> When moving code aroung, we don't have to move the labels and rename
> the gotos.
> Ex:
> Let's say we have this code:
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_foo;
> 	ret = dummy()
> 	if (ret)
> 		goto free_foo;
> 
> 	...
> 
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> And, we want to move, for whatever reason, init_something() and dummy()
> before the foo allocation. We will have to change the code to:
> 
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_bar; // not free_foo anymore !
> 	ret = dummy()
> 	if (ret)
> 		goto free_bar; // ditto
> 
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 	...
> 
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> Worse, if we have to exchange bar and foo allocation, we'll also have
> to exchange the deallocation of foo and bar and change all gotos beneath :
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_foo; // not free_foo anymore
> 	ret = dummy()
> 	if (ret)
> 		goto free_foo; //ditto
> 
> 
> 	...
> 
> // oops ! we have to exchange that !
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> 
> That's why I prefer only one label and setting NULL.
> If I didn't convince you, I'll change it back to multiple labels :)

You are right it involves less changes when editing the code. But
on the other hand it is so often written like [my proposal], that it
almost becomes a coding style choice I guess. Anyway, I don't have a
strong opinion on this so I'll let you choose the best approach from
your point of view, unless you get other comments sharing my thoughts.

Thanks anyway for the cleanup :)

Cheers,
Miqu?l

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

[Richard Genoud]

Le 15/10/2020 ? 18:38, Miquel Raynal a ?crit?:
> Hi Richard,

>> That's why I prefer only one label and setting NULL.
>> If I didn't convince you, I'll change it back to multiple labels :)
> 
> You are right it involves less changes when editing the code. But
> on the other hand it is so often written like [my proposal], that it
> almost becomes a coding style choice I guess. Anyway, I don't have a
> strong opinion on this so I'll let you choose the best approach from
> your point of view, unless you get other comments sharing my thoughts.
> 
> Thanks anyway for the cleanup :)
> 
> Cheers,
> Miqu?l
> 
Hum. You're right, consistency is a good thing.
I looked around in other files, but I don't think a standard emerges.
How about I resend the series changing all the remaining functions
(there's only 3 remaining) to the "NULL/goto out" scheme ?

Regards,
Richard

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

[Miquel Raynal]

Hi Richard,

Richard Genoud <richard.genoud@posteo.net> wrote on Fri, 16 Oct 2020
14:31:53 +0200:

> Le 15/10/2020 ? 18:38, Miquel Raynal a ?crit?:
> > Hi Richard,  
> 
> >> That's why I prefer only one label and setting NULL.
> >> If I didn't convince you, I'll change it back to multiple labels :)  
> > 
> > You are right it involves less changes when editing the code. But
> > on the other hand it is so often written like [my proposal], that it
> > almost becomes a coding style choice I guess. Anyway, I don't have a
> > strong opinion on this so I'll let you choose the best approach from
> > your point of view, unless you get other comments sharing my thoughts.
> > 
> > Thanks anyway for the cleanup :)
> > 
> > Cheers,
> > Miqu?l
> >   
> Hum. You're right, consistency is a good thing.
> I looked around in other files, but I don't think a standard emerges.
> How about I resend the series changing all the remaining functions
> (there's only 3 remaining) to the "NULL/goto out" scheme ?
> 

Sure :)  Maybe you can wait a bit for more feedback before resending
the 18-patch series though.

Cheers,
Miqu?l

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

[Richard Genoud]

Le 14/10/2020 ? 10:06, Richard Genoud a ?crit?:
> 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 | 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);

I forgot to remove a sqfs_closedir(dirsp); at the begining of the sqfs_read() function.
So this could lead to a double free.
I'll add it in next version.

>   
>   	return ret;
>   }
>