[systemd-devel] [PATCH 0/4] looking at _cleanup_free_ usage

[Tom Gundersen]

On Fri, Jun 13, 2014 at 6:48 PM, Andreas Henriksson <andreas at fatal.se> wrote:
> Hello all!

Hi Andreas,

> I was recently bitten by a problem which I found the solution to in
> http://lists.freedesktop.org/archives/systemd-commits/2013-October/004421.html
>
> The second patch there made me curious to look at more occurances.

Thanks for looking at this!

> I do not have prior experience with _cleanup_free_ but as I understand
> it in general the pointer gets free()d when it goes out of scope
> and thus must be initialized before that happens to avoid memory corruption.

Correct.

> This thread contains patches for the occurences I found which seems
> to violate that.
> Please review them carefully!!!
> (I have not tried to trigger the actual codepaths.)

We should (IMHO) always initialize the _cleanup_ variables when
declaring them, even if it so happens that they would anyway later get
initialized. Introducing bugs later on is too easy other wise.

With that in mind, your patches all look good to me. So applied them
all. Thanks! (We don't do s-o-b, so dropped those).

> Feel free to also follow up with general discussion on best practices.
>
> Should CODING_STYLE also contain a blurb next to the recommendation
> to use _cleanup_free_ that reminds/warns people it must be initialized
> before it goes out of scope?

Even better, I think; always initialize when declaring.

> Should helper function patterns be changed to set *ret = NULL early
> on to try to prevent cases where it does not initialize a variable
> on error cases?
> ie.
>
> int myhelper(..., char **ret) {
>         int x, y, z;
>
>         assert(...);
>
>         // This is just for extra safety so the value always gets initialized.
>         *ret = NULL;
>
>         ...
>
>         if (x != y)
>                 return -EEXAMPLE;
>
>         ...
>
>         *ret = strdup("example");
>         return 0;
> }

No, I don't think so. We try to never touch the passed *ret (and
similar) pointers on failure. So you get *ret if and only if the whole
function succeeds.

> Is the same problem also affecting other _cleanup_whatever_ variants?
>
> Is there a general solution in the macro itself to make sure
> the variable it is used on always gets initialized?
>
> Should CODING_STYLE be changed to recommend this:
>         _cleanup_free_ char *myptr = strdup("example");
> .. instead of this:
>         _cleanup_free_ char *myptr;
>
>         myptr = strdup("example");
> ... to make it easier to spot (or grep for) errors?

We try to avoid calling functions when declaring variables, so I'd just do:

_cleanup_free_ char *myptr = NULL;

myptr = strdup("example");

> Are there any static code analyzers that can catch these errors that can
> be used to avoid relying on manual reviews catching these errors?

As far as I know, there is work to make the llvm static analyzer work
with this, but at the moment that is not ready.

Cheers,

Tom