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 <nshirokovskiy@xxxxxxxxxxxxx>
---
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 09f63d9..be9b528 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 529b340..1201567 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 1ff55e2..2861ee6 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 ce4db5d..ae3a42c 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 713307c..cfdaa74 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 d81ec60..49b74bc 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);
--
1.8.3.1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
