Re: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak

[Richard Genoud]

Hi Miquel !
Thanks for your feedback.

Le 15/10/2020 à 15:54, Miquel Raynal a écrit :

Hi Richard,

Richard Genoud <richard.gen...@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.gen...@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