[libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
ka maybe have been freeed in virObjectUnref, application using virKeepAliveTimer will segfault when unlock ka. We should keep ka's refs positive before using it. Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> --- src/rpc/virkeepalive.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index c9faf88..4f666fd 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -160,17 +160,17 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque) bool dead; void *client; + virObjectRef(ka); virObjectLock(ka); client = ka->client; dead = virKeepAliveTimerInternal(ka, &msg); + virObjectUnlock(ka); + if (!dead && !msg) goto cleanup; - virObjectRef(ka); - virObjectUnlock(ka); - if (dead) { ka->deadCB(client); } else if (ka->sendCB(client, msg) < 0) { @@ -178,11 +178,8 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque) virNetMessageFree(msg); } - virObjectLock(ka); - virObjectUnref(ka); - cleanup: - virObjectUnlock(ka); + virObjectUnref(ka); } -- 1.8.3.1
On 04/18/2017 03:55 AM, Yi Wang wrote:
ka maybe have been freeed in virObjectUnref, application using virKeepAliveTimer will segfault when unlock ka. We should keep ka's refs positive before using it.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> --- src/rpc/virkeepalive.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index c9faf88..4f666fd 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -160,17 +160,17 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque) bool dead; void *client;
+ virObjectRef(ka); virObjectLock(ka);
client = ka->client; dead = virKeepAliveTimerInternal(ka, &msg);
+ virObjectUnlock(ka); + if (!dead && !msg) goto cleanup;
- virObjectRef(ka); - virObjectUnlock(ka); - if (dead) { ka->deadCB(client); } else if (ka->sendCB(client, msg) < 0) { @@ -178,11 +178,8 @@ virKeepAliveTimer(int timer ATTRIBUTE_UNUSED, void *opaque) virNetMessageFree(msg); }
- virObjectLock(ka); - virObjectUnref(ka); - cleanup: - virObjectUnlock(ka); + virObjectUnref(ka); }
Something doesn't feel right here. Firstly, there should always be at least one reference for this callback to use obtained in virKeepAliveStart(). So we should be able to do virObjectLock(ka) safely here. If we are not, something else is messing up the ref counting. Secondly, it shouldn't matter if we do ref() followed by lock() or the other way around. The first seems racy; well decreasing chance to hit a race but nor resolving it. Do you have a reproducer or something? backtrace perhaps? We can investigate further. Michal
SGkgTWljaGFsLA0KDQoNCg0KVGhhbmtzIGZvciB5b3VyIHJldmlldy4gVGhlIHByb2JsZW0gb2Nj dXJlZCBpbiBhIHB5dGhvbiBhcHBsaWNhdGluIHVzaW5nIGxpYnZpcnQtcHl0aG9uLCB3aGljaCBo YXMgbm8gcmVmKCkuICBJZiB3ZSB1bnJlZigpIGZpcnN0IGluIHZpcktlZXBBbGl2ZVRpbWVyLCB3 ZSBtYXkgZ2V0IGEgc2VnZmF1bHQgaW4gdmlyT2JqZWN0VW5sb2NrKCkgd2hlbiBjbGVhbnVwLCBz byBJIHN1cHBvc2UgdGhhdCBteSBwYXRjaCBpcyBzYWZlciwgOi0pIEhlcmUgaXMgdGhlIGJhY2t0 cmFjZToNCg0KDQoNCg0KDQoNCg0KIzAgIDB4MDAwMDdmZDhmNzk5NzBlOCBpbiB2aXJDbGFzc0lz RGVyaXZlZEZyb20gKGtsYXNzPTB4ZGVhZGJlZWYsIHBhcmVudD0weDdmZDhlODAwMWI4MCkgYXQg dXRpbC92aXJvYmplY3QuYzoxNjkNCg0KMTY5CSAgICAgICAgaWYgKGtsYXNzLe+8nm1hZ2ljID09 IHBhcmVudC3vvJ5tYWdpYykNCg0KTWlzc2luZyBzZXBhcmF0ZSBkZWJ1Z2luZm9zLCB1c2U6IGRl YnVnaW5mby1pbnN0YWxsIHB5dGhvbi0yLjcuNS0xNi5lbDcueDg2XzY0DQoNCihnZGIpIGJ0DQoN CiMwICAweDAwMDA3ZmQ4Zjc5OTcwZTggaW4gdmlyQ2xhc3NJc0Rlcml2ZWRGcm9tIChrbGFzcz0w eGRlYWRiZWVmLCBwYXJlbnQ9MHg3ZmQ4ZTgwMDFiODApIGF0IHV0aWwvdmlyb2JqZWN0LmM6MTY5 DQoNCiMxICAweDAwMDA3ZmQ4Zjc5OTc0MmUgaW4gdmlyT2JqZWN0SXNDbGFzcyAoYW55b2JqPWFu eW9iakBlbnRyeT0weDdmZDhlODAwYjljMCwga2xhc3M977ycb3B0aW1pemVkIG91dO+8nikgYXQg dXRpbC92aXJvYmplY3QuYzozNjUNCg0KIzIgIDB4MDAwMDdmZDhmNzk5NzRlNCBpbiB2aXJPYmpl Y3RVbmxvY2sgKGFueW9iaj0weDdmZDhlODAwYjljMCkgYXQgdXRpbC92aXJvYmplY3QuYzozMzgN Cg0KIzMgIDB4MDAwMDdmZDhmN2FjNDc3ZSBpbiB2aXJLZWVwQWxpdmVUaW1lciAodGltZXI977yc b3B0aW1pemVkIG91dO+8niwgb3BhcXVlPTB4N2ZkOGU4MDBiOWMwKSBhdCBycGMvdmlya2VlcGFs aXZlLmM6MTc3DQoNCiM0ICAweDAwMDA3ZmQ4ZjdlNWM5Y2YgaW4gbGlidmlydF92aXJFdmVudElu dm9rZVRpbWVvdXRDYWxsYmFjayAoKSBmcm9tIC91c3IvbGliNjQvcHl0aG9uMi43L3NpdGUtcGFj a2FnZXMvbGlidmlydG1vZC5zbw0KDQojNSAgMHgwMDAwN2ZkOGZmNjRkYjk0IGluIFB5RXZhbF9F dmFsRnJhbWVFeCAoKSBmcm9tIC9saWI2NC9saWJweXRob24yLjcuc28uMS4wDQoNCiM2ICAweDAw MDA3ZmQ4ZmY2NGYxYWQgaW4gUHlFdmFsX0V2YWxDb2RlRXggKCkgZnJvbSAvbGliNjQvbGlicHl0 aG9uMi43LnNvLjEuMA0KDQojNyAgMHgwMDAwN2ZkOGZmNjRkODVmIGluIFB5RXZhbF9FdmFsRnJh bWVFeCAoKSBmcm9tIC9saWI2NC9saWJweXRob24yLjcuc28uMS4wDQoNCiM4ICAweDAwMDA3ZmQ4 ZmY2NGQ5NTAgaW4gUHlFdmFsX0V2YWxGcmFtZUV4ICgpIGZyb20gL2xpYjY0L2xpYnB5dGhvbjIu Ny5zby4xLjANCg0KIzkgIDB4MDAwMDdmZDhmZjY0ZDk1MCBpbiBQeUV2YWxfRXZhbEZyYW1lRXgg KCkgZnJvbSAvbGliNjQvbGlicHl0aG9uMi43LnNvLjEuMA0KDQojMTAgMHgwMDAwN2ZkOGZmNjRm MWFkIGluIFB5RXZhbF9FdmFsQ29kZUV4ICgpIGZyb20gL2xpYjY0L2xpYnB5dGhvbjIuNy5zby4x LjANCg0KIzExIDB4MDAwMDdmZDhmZjVkYzA5OCBpbiBmdW5jdGlvbl9jYWxsICgpIGZyb20gL2xp YjY0L2xpYnB5dGhvbjIuNy5zby4xLjANCg0KIzEyIDB4MDAwMDdmZDhmZjViNzA3MyBpbiBQeU9i amVjdF9DYWxsICgpIGZyb20gL2xpYjY0L2xpYnB5dGhvbjIuNy5zby4xLjANCg0KIzEzIDB4MDAw MDdmZDhmZjVjNjA4NSBpbiBpbnN0YW5jZW1ldGhvZF9jYWxsICgpIGZyb20gL2xpYjY0L2xpYnB5 dGhvbjIuNy5zby4xLjANCg0KIzE0IDB4MDAwMDdmZDhmZjViNzA3MyBpbiBQeU9iamVjdF9DYWxs ICgpIGZyb20gL2xpYjY0L2xpYnB5dGhvbjIuNy5zby4xLjANCg0KIzE1IDB4MDAwMDdmZDhmZjY0 OGZmNyBpbiBQeUV2YWxfQ2FsbE9iamVjdFdpdGhLZXl3b3JkcyAoKSBmcm9tIC9saWI2NC9saWJw eXRob24yLjcuc28uMS4wDQoNCiMxNiAweDAwMDA3ZmQ4ZmY2N2Q3ZTIgaW4gdF9ib290c3RyYXAg KCkgZnJvbSAvbGliNjQvbGlicHl0aG9uMi43LnNvLjEuMA0KDQojMTcgMHgwMDAwN2ZkOGZmMzU4 ZGYzIGluIHN0YXJ0X3RocmVhZCAoKSBmcm9tIC9saWI2NC9saWJwdGhyZWFkLnNvLjANCg0KIzE4 IDB4MDAwMDdmZDhmZTk3ZDNlZCBpbiBjbG9uZSAoKSBmcm9tIC9saWI2NC9saWJjLnNvLjYNCg0K DQoNCg0KDQoNCg0KDQoNCi0tLQ0KDQpCZXN0IHdpc2hlcw0KDQpZaSBXYW5nDQoNCg0KDQoNCg0K DQoNCk9yaWdpbmFsIE1haWwNCg0KDQoNClNlbmRlcjogIO+8nG1wcml2b3puQHJlZGhhdC5jb23v vJ4NClRvOiBXYW5nWWkxMDEyOTk2MyDvvJxsaWJ2aXItbGlzdEByZWRoYXQuY29t77yeDQpDQzog IO+8nGpkZW5lbWFyQHJlZGhhdC5jb23vvJ5MaXVKaWFuSnVuMTAwMzM0ODINCkRhdGU6IDIwMTcv MDQvMjAgMjA6NDANClN1YmplY3Q6IFJlOiBbbGlidmlydF0gW1BBVENIXSBbUEFUQ0hdIHJwYzog Zml4IGtlZXAgYWxpdmUgdGltZXIgc2VnZmF1bHQNCg0KDQoNCg0KDQpPbiAwNC8xOC8yMDE3IDAz OjU1IEFNLCBZaSBXYW5nIHdyb3RlOg0K77yeIGthIG1heWJlIGhhdmUgYmVlbiBmcmVlZWQgaW4g dmlyT2JqZWN0VW5yZWYsIGFwcGxpY2F0aW9uIHVzaW5nDQrvvJ4gdmlyS2VlcEFsaXZlVGltZXIg d2lsbCBzZWdmYXVsdCB3aGVuIHVubG9jayBrYS4gV2Ugc2hvdWxkIGtlZXANCu+8niBrYSdzIHJl ZnMgcG9zaXRpdmUgYmVmb3JlIHVzaW5nIGl0Lg0K77yeDQrvvJ4gU2lnbmVkLW9mZi1ieTogWWkg V2FuZyDvvJx3YW5nLnlpNTlAenRlLmNvbS5jbu+8ng0K77yeIC0tLQ0K77yeICBzcmMvcnBjL3Zp cmtlZXBhbGl2ZS5jIHwgMTEgKysrKy0tLS0tLS0NCu+8niAgMSBmaWxlIGNoYW5nZWQsIDQgaW5z ZXJ0aW9ucygrKSwgNyBkZWxldGlvbnMoLSkNCu+8ng0K77yeIGRpZmYgLS1naXQgYS9zcmMvcnBj L3ZpcmtlZXBhbGl2ZS5jIGIvc3JjL3JwYy92aXJrZWVwYWxpdmUuYw0K77yeIGluZGV4IGM5ZmFm ODguLjRmNjY2ZmQgMTAwNjQ0DQrvvJ4gLS0tIGEvc3JjL3JwYy92aXJrZWVwYWxpdmUuYw0K77ye ICsrKyBiL3NyYy9ycGMvdmlya2VlcGFsaXZlLmMNCu+8niBAQCAtMTYwLDE3ICsxNjAsMTcgQEAg dmlyS2VlcEFsaXZlVGltZXIoaW50IHRpbWVyIEFUVFJJQlVURV9VTlVTRUQsIHZvaWQgKm9wYXF1 ZSkNCu+8niAgICAgIGJvb2wgZGVhZA0K77yeICAgICAgdm9pZCAqY2xpZW50DQrvvJ4NCu+8niAr ICAgIHZpck9iamVjdFJlZihrYSkNCu+8niAgICAgIHZpck9iamVjdExvY2soa2EpDQrvvJ4NCu+8 niAgICAgIGNsaWVudCA9IGthLe+8nmNsaWVudA0K77yeICAgICAgZGVhZCA9IHZpcktlZXBBbGl2 ZVRpbWVySW50ZXJuYWwoa2EsICZtc2cpDQrvvJ4NCu+8niArICAgIHZpck9iamVjdFVubG9jayhr YSkNCu+8niArDQrvvJ4gICAgICBpZiAoIWRlYWQgJiYgIW1zZykNCu+8niAgICAgICAgICBnb3Rv IGNsZWFudXANCu+8ng0K77yeIC0gICAgdmlyT2JqZWN0UmVmKGthKQ0K77yeIC0gICAgdmlyT2Jq ZWN0VW5sb2NrKGthKQ0K77yeIC0NCu+8niAgICAgIGlmIChkZWFkKSB7DQrvvJ4gICAgICAgICAg a2Et77yeZGVhZENCKGNsaWVudCkNCu+8niAgICAgIH0gZWxzZSBpZiAoa2Et77yec2VuZENCKGNs aWVudCwgbXNnKSDvvJwgMCkgew0K77yeIEBAIC0xNzgsMTEgKzE3OCw4IEBAIHZpcktlZXBBbGl2 ZVRpbWVyKGludCB0aW1lciBBVFRSSUJVVEVfVU5VU0VELCB2b2lkICpvcGFxdWUpDQrvvJ4gICAg ICAgICAgdmlyTmV0TWVzc2FnZUZyZWUobXNnKQ0K77yeICAgICAgfQ0K77yeDQrvvJ4gLSAgICB2 aXJPYmplY3RMb2NrKGthKQ0K77yeIC0gICAgdmlyT2JqZWN0VW5yZWYoa2EpDQrvvJ4gLQ0K77ye ICAgY2xlYW51cDoNCu+8niAtICAgIHZpck9iamVjdFVubG9jayhrYSkNCu+8niArICAgIHZpck9i amVjdFVucmVmKGthKQ0K77yeICB9DQrvvJ4NCu+8ng0K77yeDQoNClNvbWV0aGluZyBkb2Vzbid0 IGZlZWwgcmlnaHQgaGVyZS4gRmlyc3RseSwgdGhlcmUgc2hvdWxkIGFsd2F5cyBiZSBhdCANCmxl YXN0IG9uZSByZWZlcmVuY2UgZm9yIHRoaXMgY2FsbGJhY2sgdG8gdXNlIG9idGFpbmVkIGluIA0K dmlyS2VlcEFsaXZlU3RhcnQoKS4gU28gd2Ugc2hvdWxkIGJlIGFibGUgdG8gZG8gdmlyT2JqZWN0 TG9jayhrYSkgc2FmZWx5IA0KaGVyZS4gSWYgd2UgYXJlIG5vdCwgc29tZXRoaW5nIGVsc2UgaXMg bWVzc2luZyB1cCB0aGUgcmVmIGNvdW50aW5nLg0KDQpTZWNvbmRseSwgaXQgc2hvdWxkbid0IG1h dHRlciBpZiB3ZSBkbyByZWYoKSBmb2xsb3dlZCBieSBsb2NrKCkgb3IgdGhlIA0Kb3RoZXIgd2F5 IGFyb3VuZC4gVGhlIGZpcnN0IHNlZW1zIHJhY3kgd2VsbCBkZWNyZWFzaW5nIGNoYW5jZSB0byBo aXQgYSANCnJhY2UgYnV0IG5vciByZXNvbHZpbmcgaXQuDQoNCkRvIHlvdSBoYXZlIGEgcmVwcm9k dWNlciBvciBzb21ldGhpbmc/IGJhY2t0cmFjZSBwZXJoYXBzPyBXZSBjYW4gDQppbnZlc3RpZ2F0 ZSBmdXJ0aGVyLg0KDQpNaWNoYWw=
On 04/24/2017 05:31 AM, wang.yi59@zte.com.cn wrote:
Hi Michal,
Thanks for your review. The problem occured in a python applicatin using libvirt-python, which has no ref(). If we unref() first in virKeepAliveTimer, we may get a segfault in virObjectUnlock() when cleanup, so I suppose that my patch is safer, :-) Here is the backtrace:
Okay, now it makes more sense. However, the ref() I was referring to is our libvirt virObjectRef() not the one in python. The code where segfault occurs is too deep for python to see anyway. One thing that I am still worried about is doing virObjectRef() before virObjectLock(). I think these two steps should be swapped. Can you check please whether that also fix your problem when you switch those two lines? Michal
On Mon, Apr 24, 2017 at 11:15:31AM +0200, Michal Privoznik wrote:
On 04/24/2017 05:31 AM, wang.yi59@zte.com.cn wrote:
Hi Michal,
Thanks for your review. The problem occured in a python applicatin using libvirt-python, which has no ref(). If we unref() first in virKeepAliveTimer, we may get a segfault in virObjectUnlock() when cleanup, so I suppose that my patch is safer, :-) Here is the backtrace:
Okay, now it makes more sense. However, the ref() I was referring to is our libvirt virObjectRef() not the one in python. The code where segfault occurs is too deep for python to see anyway. One thing that I am still worried about is doing virObjectRef() before virObjectLock(). I think these two steps should be swapped.
That ordering should not matter. At the time this code runs, we *must* be guaranteed that we already hold a valid ref on the object. If we did not have that existing ref, then both virObjectRef and virObjectLock could segv. So the 'virObjectRef' must be acquiring a second reference, and thus doing lock first would not give us any further protection 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 :|
On 04/18/2017 03:55 AM, Yi Wang wrote:
ka maybe have been freeed in virObjectUnref, application using virKeepAliveTimer will segfault when unlock ka. We should keep ka's refs positive before using it.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> --- src/rpc/virkeepalive.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
So after discussion in the other thread [1], I've squashed in the backtrace you posted there, ACKed and pushed. Congratulations on your first libvirt contribution! Michal 1: https://www.redhat.com/archives/libvir-list/2017-April/msg01024.html
participants (4)
-
Daniel P. Berrange -
Michal Privoznik -
wang.yi59＠zte.com.cn -
Yi Wang