| Message ID | 20201014080622.14970-6-richard.genoud@posteo.net |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs/squashfs: fix memory leaks and introduce exists() function | expand |
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
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;
*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(-)