migration/migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Since socket_parse() will allocate memory for 'saddr',and its value
will pass to 'addr' that allocated by migrate_uri_parse(),
then 'saddr' will no longer used,need to free.
But due to 'saddr->u' is shallow copying the contents of the union,
the members of this union containing allocated strings,and will be used after that.
So just free 'saddr' itself without doing a deep free on the contents of the SocketAddress.
Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'")
Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
---
migration/migration.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..1832dad618 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -462,7 +462,6 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
{
g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
- SocketAddress *saddr = NULL;
InetSocketAddress *isock = &addr->u.rdma;
strList **tail = &addr->u.exec.args;
@@ -487,12 +486,14 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
strstart(uri, "vsock:", NULL) ||
strstart(uri, "fd:", NULL)) {
addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
- saddr = socket_parse(uri, errp);
+ SocketAddress *saddr = socket_parse(uri, errp);
if (!saddr) {
return false;
}
addr->u.socket.type = saddr->type;
addr->u.socket.u = saddr->u;
+ /* Don't free the objects inside; their ownership moved to "addr" */
+ g_free(saddr);
} else if (strstart(uri, "file:", NULL)) {
addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
addr->u.file.filename = g_strdup(uri + strlen("file:"));
--
2.34.1On Mon, Nov 20, 2023 at 11:14:28AM +0800, Zongmin Zhou wrote:
> Since socket_parse() will allocate memory for 'saddr',and its value
> will pass to 'addr' that allocated by migrate_uri_parse(),
> then 'saddr' will no longer used,need to free.
> But due to 'saddr->u' is shallow copying the contents of the union,
> the members of this union containing allocated strings,and will be used after that.
> So just free 'saddr' itself without doing a deep free on the contents of the SocketAddress.
>
> Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'")
> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
Ping? Has this been forgotten?
Best regards!
On 2023/11/20 22:01, Peter Xu wrote:
> On Mon, Nov 20, 2023 at 11:14:28AM +0800, Zongmin Zhou wrote:
>> Since socket_parse() will allocate memory for 'saddr',and its value
>> will pass to 'addr' that allocated by migrate_uri_parse(),
>> then 'saddr' will no longer used,need to free.
>> But due to 'saddr->u' is shallow copying the contents of the union,
>> the members of this union containing allocated strings,and will be used after that.
>> So just free 'saddr' itself without doing a deep free on the contents of the SocketAddress.
>>
>> Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'")
>> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
On Wed, Nov 29, 2023 at 10:09:12AM +0800, Zongmin Zhou wrote: > Ping? Has this been forgotten? Juan, Would you send a pull to include this? Thanks! -- Peter Xu
On 29/11/23 8:17 pm, Peter Xu wrote: > On Wed, Nov 29, 2023 at 10:09:12AM +0800, Zongmin Zhou wrote: >> Ping? Has this been forgotten? > Juan, > > Would you send a pull to include this? > > Thanks! > Peter, Juan - Could you also pull a similar patch related to memory leak together, it has been tested and reviewed by Markus already. ref - https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05948.htm. TIA ! Regards, Het Gala
On Mon, Nov 20, 2023 at 11:14:28AM +0800, Zongmin Zhou wrote:
> Since socket_parse() will allocate memory for 'saddr',and its value
> will pass to 'addr' that allocated by migrate_uri_parse(),
> then 'saddr' will no longer used,need to free.
> But due to 'saddr->u' is shallow copying the contents of the union,
> the members of this union containing allocated strings,and will be used after that.
> So just free 'saddr' itself without doing a deep free on the contents of the SocketAddress.
>
> Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'")
> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> ---
> migration/migration.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
and we want this fix in the next -rc release, since the memleak is a regression.
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 28a34c9068..1832dad618 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -462,7 +462,6 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
> {
> g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
> g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> - SocketAddress *saddr = NULL;
> InetSocketAddress *isock = &addr->u.rdma;
> strList **tail = &addr->u.exec.args;
>
> @@ -487,12 +486,14 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
> strstart(uri, "vsock:", NULL) ||
> strstart(uri, "fd:", NULL)) {
> addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> - saddr = socket_parse(uri, errp);
> + SocketAddress *saddr = socket_parse(uri, errp);
> if (!saddr) {
> return false;
> }
> addr->u.socket.type = saddr->type;
> addr->u.socket.u = saddr->u;
> + /* Don't free the objects inside; their ownership moved to "addr" */
> + g_free(saddr);
> } else if (strstart(uri, "file:", NULL)) {
> addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
> addr->u.file.filename = g_strdup(uri + strlen("file:"));
> --
> 2.34.1
>
With 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 :|
© 2016 - 2024 Red Hat, Inc.