[libvirt] [PATCH] bridge: leases: Fix potential crash caused by use after free
Don't free individual JSON array members as the array will be freed at the end. This may potentially lead to a crash although it didn't crash on my setup. --- src/network/bridge_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d5577e0..f3aff1c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3437,10 +3437,8 @@ networkGetDHCPLeasesHelper(virNetworkObjPtr obj, goto error; } - if (mac && virMacAddrCompare(mac, mac_tmp)) { - virJSONValueFree(lease_tmp); + if (mac && virMacAddrCompare(mac, mac_tmp)) continue; - } if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0) { /* A lease cannot be present without expiry-time */ -- 1.9.3
On 06/24/14 13:54, Peter Krempa wrote:
Don't free individual JSON array members as the array will be freed at the end. This may potentially lead to a crash although it didn't crash on my setup. --- src/network/bridge_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
It crashed now in valgrind: ==2487543== Process terminating with default action of signal 11 (SIGSEGV): dumping core ==2487543== Access not within mapped region at address 0x0 ==2487543== at 0x52ADAF7: virFree (viralloc.c:582) ==2487543== by 0x52E76D3: virJSONValueFree (virjson.c:76) ==2487543== by 0x52E773F: virJSONValueFree (virjson.c:83) ==2487543== by 0x1317A8F8: networkGetDHCPLeasesHelper (bridge_driver.c:3533) ==2487543== by 0x1317ABFE: networkGetDHCPLeasesForMAC (bridge_driver.c:3586) ==2487543== by 0x541D2E1: virNetworkGetDHCPLeasesForMAC (libvirt.c:21154) ==2487543== by 0x159082: remoteDispatchNetworkGetDHCPLeasesForMAC (remote.c:6347) ==2487543== by 0x13D0B7: remoteDispatchNetworkGetDHCPLeasesForMACHelper (remote_dispatch.h:10355) ==2487543== by 0x547B0D1: virNetServerProgramDispatchCall (virnetserverprogram.c:437) ==2487543== by 0x547AC2E: virNetServerProgramDispatch (virnetserverprogram.c:307) ==2487543== by 0x170443: virNetServerProcessMsg (virnetserver.c:172) ==2487543== by 0x170529: virNetServerHandleJob (virnetserver.c:193) I was apparently lucky before and the pointers mapped to memory that was still mapped. Peter
On Tue, Jun 24, 2014 at 01:54:42PM +0200, Peter Krempa wrote:
Don't free individual JSON array members as the array will be freed at the end. This may potentially lead to a crash although it didn't crash on my setup. --- src/network/bridge_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d5577e0..f3aff1c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3437,10 +3437,8 @@ networkGetDHCPLeasesHelper(virNetworkObjPtr obj, goto error; }
- if (mac && virMacAddrCompare(mac, mac_tmp)) { - virJSONValueFree(lease_tmp); + if (mac && virMacAddrCompare(mac, mac_tmp)) continue; - }
if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0) { /* A lease cannot be present without expiry-time */
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 :|
On 06/24/14 14:23, Daniel P. Berrange wrote:
On Tue, Jun 24, 2014 at 01:54:42PM +0200, Peter Krempa wrote:
Don't free individual JSON array members as the array will be freed at the end. This may potentially lead to a crash although it didn't crash on my setup. --- src/network/bridge_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d5577e0..f3aff1c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3437,10 +3437,8 @@ networkGetDHCPLeasesHelper(virNetworkObjPtr obj, goto error; }
- if (mac && virMacAddrCompare(mac, mac_tmp)) { - virJSONValueFree(lease_tmp); + if (mac && virMacAddrCompare(mac, mac_tmp)) continue; - }
if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0) { /* A lease cannot be present without expiry-time */
ACK
Pushed; Thanks. Peter
participants (2)
-
Daniel P. Berrange -
Peter Krempa