[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 at 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 at 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