8:55 p.m.
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(a)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
7:39 a.m.
New subject: [libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
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(a)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
11:31 a.m.
New subject: [libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
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=
4:15 a.m.
New subject: [libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
On 04/24/2017 05:31 AM, wang.yi59(a)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
4:24 a.m.
New subject: [libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
On Mon, Apr 24, 2017 at 11:15:31AM +0200, Michal Privoznik wrote:
On 04/24/2017 05:31 AM, wang.yi59(a)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 :|
5:23 a.m.
New subject: [libvirt] [PATCH] [PATCH] rpc: fix keep alive timer segfault
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(a)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
2769
days inactive
2775
days old
5 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Michal Privoznik -
wang.yi59＠zte.com.cn -
Yi Wang