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 wrote on Wed, 14 Oct 2020
10:06:10 +0200:
> *file and *dir were not freed on error
>
> Signed-off-by: Richard Genoud
> ---
> 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
