| Message ID | 20201014080622.14970-7-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, Richard Genoud <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020 10:06:11 +0200: > pos_list wasn't freed on every error > > Signed-off-by: Richard Genoud <richard.genoud@posteo.net> 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(&sblk->fragment_table_start) - > get_unaligned_le64(&sblk->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, &compressed, &src_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) > &dest_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
Hi Miquel ! Thanks for your feedback. Le 15/10/2020 à 15:54, Miquel Raynal a écrit : > Hi Richard, > > Richard Genoud <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020 > 10:06:11 +0200: > >> pos_list wasn't freed on every error >> >> Signed-off-by: Richard Genoud <richard.genoud@posteo.net> > > 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(&sblk->fragment_table_start) - >> get_unaligned_le64(&sblk->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, &compressed, &src_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) >> &dest_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 > Thanks ! Richard
Hi Richard, Richard Genoud <richard.genoud@posteo.net> 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 <richard.genoud@posteo.net> wrote on Wed, 14 Oct 2020 > > 10:06:11 +0200: > > > >> pos_list wasn't freed on every error > >> > >> Signed-off-by: Richard Genoud <richard.genoud@posteo.net> > > > > 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
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
Hi Richard, Richard Genoud <richard.genoud@posteo.net> 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
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(&sblk->fragment_table_start) - get_unaligned_le64(&sblk->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, &compressed, &src_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) &dest_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;
pos_list wasn't freed on every error Signed-off-by: Richard Genoud <richard.genoud@posteo.net> --- fs/squashfs/sqfs.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)