On Thu, Oct 18, 2012 at 16:44:24 +0200, Michal Privoznik wrote:

which frees all allocated memory but doesn't set the passed pointer to NULL. Therefore, we must do it ourselves. This is causing actual libvirtd crash: Basically, when doing 'virsh net-edit' the newDef should be dropped. And the memory is freed, indeed. However, the pointer is not set to NULL but kept instead. And the next duo of calls 'virsh net-start' and 'virsh net-destroy' starts the disaster. The latter one does the same as 'virsh destroy'; it sees that newDef is nonNULL so it replaces def with newDef (which has been freed already as said a few lines above). Therefore any subsequent call accessing def will hit the ground. --- src/conf/network_conf.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 891d48c..0f7470d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -260,8 +260,9 @@ virNetworkObjAssignDef(virNetworkObjPtr network, return -1; } } else if (!live) { - virNetworkDefFree(network->newDef); /* should be unnecessary */ + virNetworkDefFree(network->newDef); virNetworkDefFree(network->def); + network->newDef = NULL; network->def = def; } else { virReportError(VIR_ERR_OPERATION_INVALID,

ACK Jirka

On 10/18/2012 10:44 AM, Michal Privoznik wrote:

which frees all allocated memory but doesn't set the passed pointer to NULL. Therefore, we must do it ourselves. This is causing actual libvirtd crash: Basically, when doing 'virsh net-edit' the newDef should be dropped.

Well, when you're doing virsh net-edit, if the network is active, you should be updating obj->newDef, and if the network is inactive, you should be updating obj->def (and in that case, obj->newDef should have already been NULL). So I'm not sure what you mean by "newDef should be dropped" here.

And the memory is freed, indeed. However, the pointer is not set to NULL but kept instead. And the next duo of calls 'virsh net-start' and 'virsh net-destroy' starts the disaster.

Interesting, I just started hitting crashes this morning in the opposite order - if I start a network and that start fails, then I try to edit it, it will crash during the GetXMLDesc() at the beginning of the edit. I'll retry now with your patch to make sure it also fixes this scenario (sounds like it will). However, I think something is still broken in the logic. The way this is *supposed* to work (and the reason for the "should be unnecessary" comment you removed) is that newDef should always be NULL it the network isn't active, and should always be non-null if it is active. Apparently that rule is being broken (in spite of that, your patch is of course still correct :-)

The latter one does the same as 'virsh destroy'; it sees that newDef is nonNULL so it replaces def with newDef (which has been freed already as said a few lines above). Therefore any subsequent call accessing def will hit the ground. --- src/conf/network_conf.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 891d48c..0f7470d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -260,8 +260,9 @@ virNetworkObjAssignDef(virNetworkObjPtr network, return -1; } } else if (!live) { - virNetworkDefFree(network->newDef); /* should be unnecessary */ + virNetworkDefFree(network->newDef); virNetworkDefFree(network->def); + network->newDef = NULL; network->def = def; } else { virReportError(VIR_ERR_OPERATION_INVALID,