[PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
From: jiangjiacheng <jiangjiacheng@huawei.com> In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free. In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory. Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); + if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData); diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; - if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg); goto error; + } - if (virIdentitySetCurrent(identity) < 0) + if (virIdentitySetCurrent(identity) < 0) { + xdr_free(dispatcher->arg_filter, arg); goto error; + } /* * When the RPC handler is called: @@ -427,8 +431,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, */ rv = (dispatcher->func)(server, client, msg, &rerr, arg, ret); - if (virIdentitySetCurrent(NULL) < 0) + if (virIdentitySetCurrent(NULL) < 0) { + xdr_free(dispatcher->arg_filter, arg); goto error; + } /* * If rv == 1, this indicates the dispatch func has -- 2.27.0
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client);
+ if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by: client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error;
- if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter: xdr_free(dispatcher->ret_filter, ret); and in the rest of the hunks too. But at this point, I wonder whether we shouldn't have just two places where this free is done: one in successful path and one under error label. Michal
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client);
+ if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error;
- if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret). Thanks, Peng
and in the rest of the hunks too. But at this point, I wonder whether we shouldn't have just two places where this free is done: one in successful path and one under error label.
Michal
On 10/1/22 05:35, Peng Liang wrote:
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); + if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; - if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret).
Oh, you're right. We need to call it only after dispatcher->func() was called. That is when call to virIdentitySetCurrent(NULL) fails. Because at that point dispatcher->func() already populated ret. Michal
On 10/03/2022 15:38, Michal Prívozník wrote:
On 10/1/22 05:35, Peng Liang wrote:
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); + if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; - if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret).
Oh, you're right. We need to call it only after dispatcher->func() was called. That is when call to virIdentitySetCurrent(NULL) fails. Because at that point dispatcher->func() already populated ret.
But there are some `goto error` before virIdentitySetCurrent(NULL) fails originally, e.g., [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... Since dispatcher is allocated already, xdr_free(dispatcher->ret_filter, ret) will be called if we goto error from those scenarios after applying Patch v3 (https://listman.redhat.com/archives/libvir-list/2022-September/234558.html). Will this cause invalid memory access? Thanks, Peng
Michal
On 10/4/22 12:24, Peng Liang wrote:
On 10/03/2022 15:38, Michal Prívozník wrote:
On 10/1/22 05:35, Peng Liang wrote:
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); + if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; - if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret).
Oh, you're right. We need to call it only after dispatcher->func() was called. That is when call to virIdentitySetCurrent(NULL) fails. Because at that point dispatcher->func() already populated ret.
But there are some `goto error` before virIdentitySetCurrent(NULL) fails originally, e.g., [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... Since dispatcher is allocated already, xdr_free(dispatcher->ret_filter, ret) will be called if we goto error from those scenarios after applying Patch v3 (https://listman.redhat.com/archives/libvir-list/2022-September/234558.html). Will this cause invalid memory access?
Ah, xdr_free() does not accept NULL. So we need something like this: if (dispatcher) { if (arg) xdr_free(dispatcher->arg_filter, arg); if (ret) xdr_free(dispatcher->ret_filter, ret); } Michal
On 10/04/2022 19:43, Michal Prívozník wrote:
On 10/4/22 12:24, Peng Liang wrote:
On 10/03/2022 15:38, Michal Prívozník wrote:
On 10/1/22 05:35, Peng Liang wrote:
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote:
From: jiangjiacheng <jiangjiacheng@huawei.com>
In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point to a certain struct depended on the dispatcher, so I think it's the memory of the struct's member that leaks and this memory shuld be freed by xdr_free.
In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx) into virNetServerClientDispose() to release the memory.
Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 2 ++ src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..30f6af7be5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); + if (client->rx) + virNetMessageFree(client->rx); if (client->privateData) client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; - if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret).
Oh, you're right. We need to call it only after dispatcher->func() was called. That is when call to virIdentitySetCurrent(NULL) fails. Because at that point dispatcher->func() already populated ret.
But there are some `goto error` before virIdentitySetCurrent(NULL) fails originally, e.g., [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... Since dispatcher is allocated already, xdr_free(dispatcher->ret_filter, ret) will be called if we goto error from those scenarios after applying Patch v3 (https://listman.redhat.com/archives/libvir-list/2022-September/234558.html). Will this cause invalid memory access?
Ah, xdr_free() does not accept NULL. So we need something like this:
if (dispatcher) { if (arg) xdr_free(dispatcher->arg_filter, arg); if (ret) xdr_free(dispatcher->ret_filter, ret); }
If we reach [2],[3],[4], however, then ret is allocated (but is not populated). I think there is still a invalid memory access via xdr_free(dispatcher->ret_filter, ret). Maybe the following will work? free_ret: xdr_free(dispatcher->arg_filter, ret); free_arg: xdr_free(dispatcher->arg_filter, arg); then change all gotos after (dispatcher->func)(server, client, msg, &rerr, arg, ret) to goto free_ret, and change [2], [3], [4] to goto free_arg.
Michal
On 10/4/22 13:50, Peng Liang wrote:
On 10/04/2022 19:43, Michal Prívozník wrote:
On 10/4/22 12:24, Peng Liang wrote:
On 10/03/2022 15:38, Michal Prívozník wrote:
On 10/1/22 05:35, Peng Liang wrote:
On 09/29/2022 21:31, Michal Prívozník wrote:
On 9/27/22 17:38, Jiang Jiacheng wrote: > From: jiangjiacheng <jiangjiacheng@huawei.com> > > In virNetServerProgramDispatchCall, The arg is passed as a void* and > used to point > to a certain struct depended on the dispatcher, so I think it's the > memory of the > struct's member that leaks and this memory shuld be freed by > xdr_free. > > In virNetServerClientNew, client->rx is assigned by invoking > virNetServerClientNew, > but isn't freed if client->privateData's initialization failed, > which > leads to a > memory leak. Thanks to Liang Peng's suggestion, put > virNetMessageFree(client->rx) > into virNetServerClientDispose() to release the memory. > > Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> > --- > src/rpc/virnetserverclient.c | 2 ++ > src/rpc/virnetserverprogram.c | 12 +++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/src/rpc/virnetserverclient.c > b/src/rpc/virnetserverclient.c > index a7d2dfa795..30f6af7be5 100644 > --- a/src/rpc/virnetserverclient.c > +++ b/src/rpc/virnetserverclient.c > @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj) > PROBE(RPC_SERVER_CLIENT_DISPOSE, > "client=%p", client); > + if (client->rx) > + virNetMessageFree(client->rx); > if (client->privateData) > client->privateDataFreeFunc(client->privateData);
Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
client = virNetServerClientNew(...); virObjectUnref(client);
> diff --git a/src/rpc/virnetserverprogram.c > b/src/rpc/virnetserverprogram.c > index 3ddf9f0428..a813e821a3 100644 > --- a/src/rpc/virnetserverprogram.c > +++ b/src/rpc/virnetserverprogram.c > @@ -409,11 +409,15 @@ > virNetServerProgramDispatchCall(virNetServerProgram *prog, > if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, > arg) < 0) > goto error; > - if (!(identity = virNetServerClientGetIdentity(client))) > + if (!(identity = virNetServerClientGetIdentity(client))) { > + xdr_free(dispatcher->arg_filter, arg);
But here I believe we also need to free dispatcher->ret_filter:
xdr_free(dispatcher->ret_filter, ret);
Hi Michal, I'm not sure why we need to free ret here. IIRC, xdr_free is to free memory pointed by *ret instead of ret. For example, if ret is pointing to a struct which contains a string, like: struct Test { char *str; } then I think xdr_free(dispatcher->ret_filter, ret) will free str instead of struct Test itself. So I think we will only need to call xdr_free(dispatcher->ret_filter, ret) after we call (dispatcher->func)(server, client, msg, &rerr, arg, ret).
Oh, you're right. We need to call it only after dispatcher->func() was called. That is when call to virIdentitySetCurrent(NULL) fails. Because at that point dispatcher->func() already populated ret.
But there are some `goto error` before virIdentitySetCurrent(NULL) fails originally, e.g., [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... Since dispatcher is allocated already, xdr_free(dispatcher->ret_filter, ret) will be called if we goto error from those scenarios after applying Patch v3 (https://listman.redhat.com/archives/libvir-list/2022-September/234558.html). Will this cause invalid memory access?
Ah, xdr_free() does not accept NULL. So we need something like this:
if (dispatcher) { if (arg) xdr_free(dispatcher->arg_filter, arg); if (ret) xdr_free(dispatcher->ret_filter, ret); }
If we reach [2],[3],[4], however, then ret is allocated (but is not populated). I think there is still a invalid memory access via xdr_free(dispatcher->ret_filter, ret). Maybe the following will work? free_ret: xdr_free(dispatcher->arg_filter, ret); free_arg: xdr_free(dispatcher->arg_filter, arg);
then change all gotos after (dispatcher->func)(server, client, msg, &rerr, arg, ret) to goto free_ret, and change [2], [3], [4] to goto free_arg.
When I try to simulate this problem with the following diff I don't see any invalid reads. Do you? diff --git i/src/rpc/virnetserverprogram.c w/src/rpc/virnetserverprogram.c index 212e0d5ab5..3fa88fafda 100644 --- i/src/rpc/virnetserverprogram.c +++ w/src/rpc/virnetserverprogram.c @@ -409,6 +409,8 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error; + if (rand()%10 == 0) goto error; + if (!(identity = virNetServerClientGetIdentity(client))) goto error; @@ -476,8 +478,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, error: if (dispatcher) { - xdr_free(dispatcher->arg_filter, arg); - xdr_free(dispatcher->ret_filter, ret); + if (arg) + xdr_free(dispatcher->arg_filter, arg); + if (ret) + xdr_free(dispatcher->ret_filter, ret); } /* Bad stuff (de-)serializing message, but we have an Michal
participants (3)
-
Jiang Jiacheng -
Michal Prívozník -
Peng Liang