[libvirt] [PATCH] bhyve: fix virObjectUnlock() usage
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like: virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance * src/bhyve/bhyve_driver.c (bhyveDomainGetInfo) (bhyveDomainGetState, bhyveDomainGetAutostart) (bhyveDomainSetAutostart, bhyveDomainIsActive) (bhyveDomainIsPersistent, bhyveDomainGetXMLDesc) (bhyveDomainUndefine, bhyveDomainLookupByUUID) (bhyveDomainLookupByName, bhyveDomainLookupByID) (bhyveDomainCreateWithFlags, bhyveDomainOpenConsole): Check if arg is not NULL before calling virObjectUnlock on it. --- src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 580b124..c8902bf 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -309,7 +309,8 @@ bhyveDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) ret = 0; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -334,7 +335,8 @@ bhyveDomainGetState(virDomainPtr domain, ret = 0; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -354,7 +356,8 @@ bhyveDomainGetAutostart(virDomainPtr domain, int *autostart) ret = 0; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -417,7 +420,8 @@ bhyveDomainSetAutostart(virDomainPtr domain, int autostart) cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -436,7 +440,8 @@ bhyveDomainIsActive(virDomainPtr domain) ret = virDomainObjIsActive(obj); cleanup: - virObjectUnlock(obj); + if (obj) + virObjectUnlock(obj); return ret; } @@ -455,7 +460,8 @@ bhyveDomainIsPersistent(virDomainPtr domain) ret = obj->persistent; cleanup: - virObjectUnlock(obj); + if (obj) + virObjectUnlock(obj); return ret; } @@ -474,7 +480,8 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) ret = virDomainDefFormat(vm->def, flags); cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -562,7 +569,8 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -730,7 +738,8 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom->id = vm->def->id; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return dom; } @@ -757,7 +766,8 @@ static virDomainPtr bhyveDomainLookupByName(virConnectPtr conn, dom->id = vm->def->id; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return dom; } @@ -785,7 +795,8 @@ bhyveDomainLookupByID(virConnectPtr conn, dom->id = vm->def->id; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return dom; } @@ -820,7 +831,8 @@ bhyveDomainCreateWithFlags(virDomainPtr dom, start_flags); cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -960,7 +972,8 @@ bhyveDomainOpenConsole(virDomainPtr dom, ret = 0; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return ret; } -- 1.9.0
On Sat, May 17, 2014 at 11:44:48PM +0400, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
* src/bhyve/bhyve_driver.c (bhyveDomainGetInfo) (bhyveDomainGetState, bhyveDomainGetAutostart) (bhyveDomainSetAutostart, bhyveDomainIsActive) (bhyveDomainIsPersistent, bhyveDomainGetXMLDesc) (bhyveDomainUndefine, bhyveDomainLookupByUUID) (bhyveDomainLookupByName, bhyveDomainLookupByID) (bhyveDomainCreateWithFlags, bhyveDomainOpenConsole): Check if arg is not NULL before calling virObjectUnlock on it. --- src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Sat, May 17, 2014 at 11:44:48PM +0400, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
* src/bhyve/bhyve_driver.c (bhyveDomainGetInfo) (bhyveDomainGetState, bhyveDomainGetAutostart) (bhyveDomainSetAutostart, bhyveDomainIsActive) (bhyveDomainIsPersistent, bhyveDomainGetXMLDesc) (bhyveDomainUndefine, bhyveDomainLookupByUUID) (bhyveDomainLookupByName, bhyveDomainLookupByID) (bhyveDomainCreateWithFlags, bhyveDomainOpenConsole): Check if arg is not NULL before calling virObjectUnlock on it. --- src/bhyve/bhyve_driver.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)
ACK
Pushed, thanks! Roman Bogorodskiy
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to gracefully handle a NULL parameter, rather than making every caller uglier? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem is this annoying warning. So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that? BTW, that's going to be a vast change, quick grep shows more than 750 calls of that function. Roman Bogorodskiy
On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
Eric Blake wrote:
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem is this annoying warning.
So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that?
Yes, since we have other virObject code that special cases NULL (for example, virObjectUnref).
BTW, that's going to be a vast change, quick grep shows more than 750 calls of that function.
It doesn't invalidate any existing caller to make virObjectUnlock() special-case NULL. And while it DOES make any existing caller that also checks for NULL be a case of redundant code, we don't have to clean all callers up in a single patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
BTW, that's going to be a vast change, quick grep shows more than 750 calls of that function.
It doesn't invalidate any existing caller to make virObjectUnlock() special-case NULL. And while it DOES make any existing caller that also checks for NULL be a case of redundant code, we don't have to clean all callers up in a single patch.
Good, so I'll add a check for NULL in virObjectUnlock() and prepare a series that cleans up the redundant if (obj) check before calling virObjectUnlock(obj), say, on per-driver basis. Roman Bogorodskiy
On Mon, May 19, 2014 at 11:12:05AM -0600, Eric Blake wrote:
On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
Eric Blake wrote:
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem is this annoying warning.
So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that?
Yes, since we have other virObject code that special cases NULL (for example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't try to paper over by ignoring. Ultimately I think the real flaw is the way we obtain the virDomainPtr pointers in the first place. ie the virDomainObjListLookup functions don't acquire a reference on the object they return. So the caller has to worry about the object being released behind their back, hence all our logic which has to set 'vm = NULL' in various places where it might have been deleted. IOW, I'd much rather we looked at changing our design here so that we didn't have so much NULL vm pointers in the first place. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Mon, May 19, 2014 at 11:12:05AM -0600, Eric Blake wrote:
On 05/19/2014 10:57 AM, Roman Bogorodskiy wrote:
Eric Blake wrote:
On 05/17/2014 01:44 PM, Roman Bogorodskiy wrote:
In a number of places in the bhyve driver, virObjectUnlock() is called with an arg without check if the arg is non-NULL, which could result in passing NULL value and a warning like:
virObjectUnlock:340 : Object 0x0 ((unknown)) is not a virObjectLockable instance
Doesn't this instead argue that we should fix virObjectUnlock to gracefully handle a NULL parameter, rather than making every caller uglier?
Calling it with NULL seems to be harmless anyway and the only problem is this annoying warning.
So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that?
Yes, since we have other virObject code that special cases NULL (for example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't try to paper over by ignoring.
Ultimately I think the real flaw is the way we obtain the virDomainPtr pointers in the first place. ie the virDomainObjListLookup functions don't acquire a reference on the object they return. So the caller has to worry about the object being released behind their back, hence all our logic which has to set 'vm = NULL' in various places where it might have been deleted.
IOW, I'd much rather we looked at changing our design here so that we didn't have so much NULL vm pointers in the first place.
Eric, could you please comment on that? Roman Bogorodskiy
On 05/21/2014 08:02 AM, Roman Bogorodskiy wrote:
So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that?
Yes, since we have other virObject code that special cases NULL (for example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't try to paper over by ignoring.
Ultimately I think the real flaw is the way we obtain the virDomainPtr pointers in the first place. ie the virDomainObjListLookup functions don't acquire a reference on the object they return. So the caller has to worry about the object being released behind their back, hence all our logic which has to set 'vm = NULL' in various places where it might have been deleted.
IOW, I'd much rather we looked at changing our design here so that we didn't have so much NULL vm pointers in the first place.
Eric, could you please comment on that?
I trust Daniel's judgment here, since he wrote virObjectPtr and virObjectUnlock. Cleaning up the DomainObjListLookup function to grab a reference will have a big ripple effect, so it probably doesn't need to be done now (that is, you don't necessarily have to be the one doing the cleanup he proposes); but I guess that means for the present that we are still stuck with the current design pattern of checking for NULL ourself before calling virObjectUnlock. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/21/2014 08:02 AM, Roman Bogorodskiy wrote:
So, what you propose is checking explicitly for NULL in virObjectUnlock before we do virObjectIsClass(), and if the passed argument is NULL indeed, just return, without logging anything about that?
Yes, since we have other virObject code that special cases NULL (for example, virObjectUnref).
IMHO passing NULL into the locking APIs is a coding error we shouldn't try to paper over by ignoring.
Ultimately I think the real flaw is the way we obtain the virDomainPtr pointers in the first place. ie the virDomainObjListLookup functions don't acquire a reference on the object they return. So the caller has to worry about the object being released behind their back, hence all our logic which has to set 'vm = NULL' in various places where it might have been deleted.
IOW, I'd much rather we looked at changing our design here so that we didn't have so much NULL vm pointers in the first place.
Eric, could you please comment on that?
I trust Daniel's judgment here, since he wrote virObjectPtr and virObjectUnlock. Cleaning up the DomainObjListLookup function to grab a reference will have a big ripple effect, so it probably doesn't need to be done now (that is, you don't necessarily have to be the one doing the cleanup he proposes); but I guess that means for the present that we are still stuck with the current design pattern of checking for NULL ourself before calling virObjectUnlock.
OK, then I'll push the current bhyve patch that was already ACKed by Daniel and leave virObjectUnlock related cleanup for now; thanks. Roman Bogorodskiy
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Roman Bogorodskiy