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.gen...@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.gen...@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
