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

[Thomas H.P. Andersen]

On Fri, Jun 13, 2014 at 7:04 PM, Tom Gundersen <teg at jklm.no> wrote:
> 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.

Compiling with clang helps to spot some of these issues but the static
analyzer could do it better. There is a llvm bug to make scan-build
understand the cleanup attribute. That should both make it warn for
this and get rid of 200+ false positives for memleaks and dead
assignments. It would make scan-build a much more useful tool for
systemd so if we have friends in llvm then please point them to this
bug:
http://llvm.org/bugs/show_bug.cgi?id=3888