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

Miquel Raynal
2020-10-15T16:38:51Z

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