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:

 Invalid read of size 8
    at 0x52CA3DA: virFree (viralloc.c:582)
    by 0x52D5272: virCgroupFree (vircgroup.c:1700)
    by 0x230CE113: qemuDomainObjPrivateDataClear (qemu_domain.c:1923)
    by 0x230CE2F3: qemuDomainObjPrivateFree (qemu_domain.c:1973)
    by 0x53922D7: virDomainObjDispose (domain_conf.c:3192)
    by 0x533B8ED: virObjectUnref (virobject.c:350)
    by 0x533BE39: virObjectFreeHashData (virobject.c:591)
    by 0x5305C23: virHashFree (virhash.c:304)
    by 0x53EAFA7: virDomainObjListDispose (virdomainobjlist.c:92)
    by 0x533B8ED: virObjectUnref (virobject.c:350)
    by 0x2315E2AE: qemuStateCleanup (qemu_driver.c:1067)
    by 0x557CFF6: virStateCleanup (libvirt.c:699)
  Address 0x29fbbdd0 is 16 bytes inside a block of size 328 free'd
    at 0x4C2E13B: free (vg_replace_malloc.c:530)
    by 0x52CA3E4: virFree (viralloc.c:582)
    by 0x52D52D4: virCgroupFree (vircgroup.c:1706)
    by 0x230CE113: qemuDomainObjPrivateDataClear (qemu_domain.c:1923)
    by 0x2311CFE9: qemuProcessStop (qemu_process.c:7144)
    by 0x2311BF1C: qemuProcessStart (qemu_process.c:6745)
    by 0x2316E634: qemuDomainObjStart (qemu_driver.c:7293)
    by 0x2316E8A2: qemuDomainCreateWithFlags (qemu_driver.c:7346)
    by 0x2316E925: qemuDomainCreate (qemu_driver.c:7365)
    by 0x5594E9F: virDomainCreate (libvirt-domain.c:6534)
    by 0x1367D1: remoteDispatchDomainCreate (remote_daemon_dispatch_stubs.h:4434)
    by 0x1366EA: remoteDispatchDomainCreateHelper (remote_daemon_dispatch_stubs.h:4410)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/lxc/lxc_process.c  | 1 +
 src/qemu/qemu_cgroup.c | 2 ++
 src/qemu/qemu_domain.c | 1 +
 src/util/vircgroup.c   | 5 +++++
 4 files changed, 9 insertions(+)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 5d8fa63c67..4d118cb6fd 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -221,6 +221,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
     if (priv->cgroup) {
         virCgroupRemove(priv->cgroup);
         virCgroupFree(priv->cgroup);
+        priv->cgroup = NULL;
     }
 
     /* Get machined to terminate the machine as it may not have cleaned it
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 8a00ffcd45..cd1e01262b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -921,6 +921,7 @@ qemuInitCgroup(virDomainObjPtr vm,
         goto done;
 
     virCgroupFree(priv->cgroup);
+    priv->cgroup = NULL;
 
     if (!vm->def->resource) {
         virDomainResourceDefPtr res;
@@ -1058,6 +1059,7 @@ qemuConnectCgroup(virDomainObjPtr vm)
         goto done;
 
     virCgroupFree(priv->cgroup);
+    priv->cgroup = NULL;
 
     if (virCgroupNewDetectMachine(vm->def->name,
                                   "qemu",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bda53814a3..bfffd3da27 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1921,6 +1921,7 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
     priv->qemuDevices = NULL;
 
     virCgroupFree(priv->cgroup);
+    priv->cgroup = NULL;
 
     virPerfFree(priv->perf);
     priv->perf = NULL;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 472a8167f5..6e2e06bae3 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1371,6 +1371,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition,
                            VIR_CGROUP_MEM_HIERACHY) < 0) {
         virCgroupRemove(*group);
         virCgroupFree(*group);
+        *group = NULL;
         return -1;
     }
 
@@ -1428,6 +1429,7 @@ virCgroupNewThread(virCgroupPtr domain,
     if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) {
         virCgroupRemove(*group);
         virCgroupFree(*group);
+        *group = NULL;
         return -1;
     }
 
@@ -1466,6 +1468,7 @@ virCgroupNewDetectMachine(const char *name,
         VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'",
                   name, drivername);
         virCgroupFree(*group);
+        *group = NULL;
         return 0;
     }
 
@@ -1566,6 +1569,7 @@ virCgroupNewMachineSystemd(const char *name,
         virErrorPtr saved = virSaveLastError();
         virCgroupRemove(*group);
         virCgroupFree(*group);
+        *group = NULL;
         if (saved) {
             virSetError(saved);
             virFreeError(saved);
@@ -1617,6 +1621,7 @@ virCgroupNewMachineManual(const char *name,
         virErrorPtr saved = virSaveLastError();
         virCgroupRemove(*group);
         virCgroupFree(*group);
+        *group = NULL;
         if (saved) {
             virSetError(saved);
             virFreeError(saved);
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

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.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jul 30, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
> 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.

Like Pavel said, we already had this discussion which also wasn't moving much.
At that time I said that having the free functions take a double pointer is the
ultimate goal which I completely agree with, however, we decided to go this way
instead in the meantime. Although we broke it this way, I still prefer Michal's
original fix rather than reverting Sukrit's change, because (even though
broken), right now we're consistent in usage and then when we switch to double
pointer Free() functions, we can use one of Pavel's suggestions, preferably the
first one.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jul 30, 2018 at 12:47:37PM +0200, Erik Skultety wrote:
> On Mon, Jul 30, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
> > 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.
> 
> Like Pavel said, we already had this discussion which also wasn't moving much.
> At that time I said that having the free functions take a double pointer is the
> ultimate goal which I completely agree with, however, we decided to go this way
> instead in the meantime. Although we broke it this way, I still prefer Michal's
> original fix rather than reverting Sukrit's change, because (even though
> broken), right now we're consistent in usage and then when we switch to double
> pointer Free() functions, we can use one of Pavel's suggestions, preferably the
> first one.

The switch from ptr, to ptr-to-a-ptr is not something we can realistically
do in a "big bang". It would have to be an incremental change across the
code base, so we're going to have inconsistency no matter what.

In any case code robustness is more important than code style, so reverting
this is still the right thing todo. 

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list