On Fri, Jun 13, 2014 at 7:04 PM, Tom Gundersen <t...@jklm.no> wrote: > On Fri, Jun 13, 2014 at 6:48 PM, Andreas Henriksson <andr...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
