From fbcb73866b2517ba0c7e34e3f1af69bea4c73d6b Mon Sep 17 00:00:00 2001 From: Nikolay Shirokovskiy Date: Thu, 7 Feb 2019 15:58:46 +0300 Subject: [PATCH] rpc: client stream: dispose private data on stream dispose If we call virStreamFinish and virStreamAbort from 2 distinct threads for example we can have access to freed memory. Because when virStreamFinish finishes for example virStreamAbort yet to be finished and it access virNetClientStreamPtr object in stream->privateData. Also it does not make sense to clear @driver field. After stream is finished/aborted it is better to have appropriate error message instead of "unsupported error". This commit reverts [1] or virNetClientStreamPtr and virStreamPtr will never be unrefed due to cyclic dependency. Before this patch we don't have leaks because all execution paths we call virStreamFinish or virStreamAbort. [1] 8b6ffe40 : virNetClientStreamNew: Track origin stream Signed-off-by: Nikolay Shirokovskiy --- src/datatypes.c | 2 ++ src/datatypes.h | 1 + src/remote/remote_driver.c | 11 ++++------- src/rpc/gendispatch.pl | 3 ++- src/rpc/virnetclientstream.c | 7 +------ src/rpc/virnetclientstream.h | 3 +-- 6 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 09f63d9e15..be9b5286aa 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -763,6 +763,8 @@ virStreamDispose(void *obj) virStreamPtr st = obj; VIR_DEBUG("release dev %p", st); + if (st->ff) + st->ff(st->privateData); virObjectUnref(st->conn); } diff --git a/src/datatypes.h b/src/datatypes.h index 529b340587..12015679f3 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -665,6 +665,7 @@ struct _virStream { virStreamDriverPtr driver; void *privateData; + virFreeCallback ff; }; /** diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1ff55e241a..2861ee68e2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5835,9 +5835,6 @@ remoteStreamCloseInt(virStreamPtr st, bool streamAbort) priv->localUses--; virNetClientRemoveStream(priv->client, privst); - virObjectUnref(privst); - st->privateData = NULL; - st->driver = NULL; remoteDriverUnlock(priv); return ret; @@ -6177,8 +6174,7 @@ remoteDomainMigratePrepareTunnel3(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (!(netst = virNetClientStreamNew(st, - priv->remoteProgram, + if (!(netst = virNetClientStreamNew(priv->remoteProgram, REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3, priv->counter, false))) @@ -6191,6 +6187,7 @@ remoteDomainMigratePrepareTunnel3(virConnectPtr dconn, st->driver = &remoteStreamDrv; st->privateData = netst; + st->ff = virObjectFreeCallback; args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; @@ -7142,8 +7139,7 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, goto cleanup; } - if (!(netst = virNetClientStreamNew(st, - priv->remoteProgram, + if (!(netst = virNetClientStreamNew(priv->remoteProgram, REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3_PARAMS, priv->counter, false))) @@ -7156,6 +7152,7 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, st->driver = &remoteStreamDrv; st->privateData = netst; + st->ff = virObjectFreeCallback; if (call(dconn, priv, 0, REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3_PARAMS, (xdrproc_t) xdr_remote_domain_migrate_prepare_tunnel3_params_args, diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ce4db5d7b7..ae3a42c4c1 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1804,7 +1804,7 @@ elsif ($mode eq "client") { if ($call->{streamflag} ne "none") { print "\n"; - print " if (!(netst = virNetClientStreamNew(st, priv->remoteProgram, $call->{constname}, priv->counter, sparse)))\n"; + print " if (!(netst = virNetClientStreamNew(priv->remoteProgram, $call->{constname}, priv->counter, sparse)))\n"; print " goto done;\n"; print "\n"; print " if (virNetClientAddStream(priv->client, netst) < 0) {\n"; @@ -1814,6 +1814,7 @@ elsif ($mode eq "client") { print "\n"; print " st->driver = &remoteStreamDrv;\n"; print " st->privateData = netst;\n"; + print " st->ff = virObjectFreeCallback;\n"; } if ($call->{ProcName} eq "SupportsFeature") { diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index 32349f0d42..f27dcbfea7 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -34,8 +34,6 @@ VIR_LOG_INIT("rpc.netclientstream"); struct _virNetClientStream { virObjectLockable parent; - virStreamPtr stream; /* Reverse pointer to parent stream */ - virNetClientProgramPtr prog; int proc; unsigned serial; @@ -133,8 +131,7 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) } -virNetClientStreamPtr virNetClientStreamNew(virStreamPtr stream, - virNetClientProgramPtr prog, +virNetClientStreamPtr virNetClientStreamNew(virNetClientProgramPtr prog, int proc, unsigned serial, bool allowSkip) @@ -147,7 +144,6 @@ virNetClientStreamPtr virNetClientStreamNew(virStreamPtr stream, if (!(st = virObjectLockableNew(virNetClientStreamClass))) return NULL; - st->stream = virObjectRef(stream); st->prog = virObjectRef(prog); st->proc = proc; st->serial = serial; @@ -167,7 +163,6 @@ void virNetClientStreamDispose(void *obj) virNetMessageFree(msg); } virObjectUnref(st->prog); - virObjectUnref(st->stream); } bool virNetClientStreamMatches(virNetClientStreamPtr st, diff --git a/src/rpc/virnetclientstream.h b/src/rpc/virnetclientstream.h index d81ec60a48..49b74bcc41 100644 --- a/src/rpc/virnetclientstream.h +++ b/src/rpc/virnetclientstream.h @@ -30,8 +30,7 @@ typedef virNetClientStream *virNetClientStreamPtr; typedef void (*virNetClientStreamEventCallback)(virNetClientStreamPtr stream, int events, void *opaque); -virNetClientStreamPtr virNetClientStreamNew(virStreamPtr stream, - virNetClientProgramPtr prog, +virNetClientStreamPtr virNetClientStreamNew(virNetClientProgramPtr prog, int proc, unsigned serial, bool allowSkip);