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

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.