| Message ID | 20231120031428.908295-1-zhouzongmin@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] migration: free 'saddr' since be no longer used | expand |
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
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>
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!
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
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:"));
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(-)