Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak
Hi Richard, Richard Genoud 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
Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak
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
Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak
Hi Richard, Richard Genoud 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 wrote on Wed, 14 Oct 2020 > > 10:06:11 +0200: > > > >> pos_list wasn't freed on every error > >> > >> Signed-off-by: Richard Genoud > > > > 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
Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak
Hi Miquel !
Thanks for your feedback.
Le 15/10/2020 à 15:54, Miquel Raynal a écrit :
Hi Richard,
Richard Genoud wrote on Wed, 14 Oct 2020
10:06:11 +0200:
pos_list wasn't freed on every error
Signed-off-by: Richard Genoud
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(>fragment_table_start) -
get_unaligned_le64(>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, , _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)
Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak
Hi Richard,
Richard Genoud wrote on Wed, 14 Oct 2020
10:06:11 +0200:
> pos_list wasn't freed on every error
>
> Signed-off-by: Richard Genoud
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(>fragment_table_start) -
> get_unaligned_le64(>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, , _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)
> _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
pos_list wasn't freed on every error
Signed-off-by: Richard Genoud
---
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(>fragment_table_start) -
get_unaligned_le64(>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, , _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)
_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;
