[libvirt] [PATCH for v4.6.0] cgroups: Don't leave stale pointers around after virCgroupFree

[Daniel P. Berrangé]

On Mon, Jul 30, 2018 at 12:30:09PM +0200, Pavel Hrdina wrote:
> On Mon, Jul 30, 2018 at 09:58:41AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 30, 2018 at 09:48:51AM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> > > > One of the attributes that original virCgroupFree() had was it
> > > > set passed pointer to NULL. For instance in the following code
> > > > the latter call would be practically a no-op:
> > > > 
> > > >   virCgroupFree(&var);
> > > >   virCgroupFree(&var);
> > > > 
> > > > However, this behaviour of the function was changed in
> > > > 0f80c71822d824 but corresponding 'var = NULL' lines were not
> > > > added leading to double free:
> > > 
> > > Sigh, can we please just revert that change. It is going in completely the
> > > oppposite of what we should be doing. We want to change more functions to
> > > take a ptr to a ptr, precisely because it avoids this double-free problem.
> 
> I agree that this change was not correct and we should revert it, the
> ultimate goal should be to have all the *Free() functions to take
> double pointer and set the variable to NULL as well.
> 
> > Even more crazy, this change was done so that VIR_DEFINE_AUTOPTR_FUNC()
> > could be used to define a free function which takes a ptr to a ptr.
> > 
> > Both of these changes should be reverted, as the previously existing
> > virCgroupFree should be used as the attribute((cleanup)) function directly
> > with no wrapper created.
> 
> We already had this discussion, there are two possibilities:
> 
> 1) as the end goal simple wrapper function defined by MACRO:
> 
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
>     static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
>     { \
>         (func)(_ptr); \
>     } \
> 
> It's a static inline so it will disappear during compilation.  As it
> may look like unnecessary code there is one benefit while using
> VIR_AUTOPTR()
> 
> 2) the second option is to have a macro that would replace
> __attribute__(cleanup()), for example:
> 
> VIR_CLEANUP(func) __attribute__(cleanup(func))
> 
> 
> If you compare the usage:
> 
>     VIR_AUTOPTR(virCgroup) group = NULL;
> 
>     VIR_CLEANUP(virCgroupFree) virCgroupPtr group = NULL;
> 
> I personally prefer the first option, it's cleaner and the line is
> shorter and it's perfectly readable even though some logic is hidden.
> The second usage has a benefit that the logic is not hidden and you can
> use any function and you don't have to define anything but the line can
> get really long and ugly as we have some really long names for Free
> functions and types.
> 
> For example:
> 
>     VIR_AUTOPTR(virDomainGraphicsDef) def = NULL;
> 
>     VIR_CLEANUP(virDomainGraphicsDefFree) virDomainGraphicsDefPtr def = NULL;
> 
> And that's not even the longest free function.
> 
> Since the wrapper defined by VIR_DEFINE_AUTOPTR_FUNC() will be expanded
> during compilation I prefer that option.  It would essentially work the
> same way is VIR_CLEANUP but with the benefit that the declaration line
> is short, nice and clean.

I'm fine with either option, as long as we keep the ptr to a ptr aspect
of the original function. Probably a slight pref for the first option.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|