| Message ID | 20231115032739.933043-1-zhouzongmin@kylinos.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: free 'saddr' since be no longer used | expand |
On Wed, Nov 15, 2023 at 11:27:39AM +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(),so free 'saddr' to avoid memory leak. > > Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'") > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn> > --- > migration/migration.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/migration/migration.c b/migration/migration.c > index 28a34c9068..30ed4bf6b6 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -493,6 +493,7 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel, > } > addr->u.socket.type = saddr->type; > addr->u.socket.u = saddr->u; 'saddr->u' is a union embedded in SocketAddress, containing: union { /* union tag is @type */ InetSocketAddressWrapper inet; UnixSocketAddressWrapper q_unix; VsockSocketAddressWrapper vsock; StringWrapper fd; } u; THis assignment is *shallow* copying the contents of the union. All the type specifics structs that are members of this union containing allocated strings, and with this shallow copy, we are stealing the pointers to these allocated strings > + qapi_free_SocketAddress(saddr); This meanwhle is doing a *deep* free of the contents of the SocketAddress, which includes all the pointers we just stole. IOW, unless I'm mistaken somehow, this is going to cause a double-free With regards, Daniel
On Wed, Nov 15, 2023 at 09:49:09AM +0000, Daniel P. Berrangé wrote: > On Wed, Nov 15, 2023 at 11:27:39AM +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(),so free 'saddr' to avoid memory leak. > > > > Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'") > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn> > > --- > > migration/migration.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index 28a34c9068..30ed4bf6b6 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -493,6 +493,7 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel, > > } > > addr->u.socket.type = saddr->type; > > addr->u.socket.u = saddr->u; > > 'saddr->u' is a union embedded in SocketAddress, containing: > > union { /* union tag is @type */ > InetSocketAddressWrapper inet; > UnixSocketAddressWrapper q_unix; > VsockSocketAddressWrapper vsock; > StringWrapper fd; > } u; > > THis assignment is *shallow* copying the contents of the union. > > All the type specifics structs that are members of this union > containing allocated strings, and with this shallow copy, we > are stealing the pointers to these allocated strings > > > > + qapi_free_SocketAddress(saddr); > > This meanwhle is doing a *deep* free of the contents of the > SocketAddress, which includes all the pointers we just stole. > > IOW, unless I'm mistaken somehow, this is going to cause a > double-free Right. I think what we need is a g_free(saddr), with a comment explaining? Or, is there better way to do that? Something like a QAPI_CLONE() but not exactly: we already have the object allocated. We want to deep copy it to the current object only on the fields but not the object itself.
diff --git a/migration/migration.c b/migration/migration.c index 28a34c9068..30ed4bf6b6 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -493,6 +493,7 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel, } addr->u.socket.type = saddr->type; addr->u.socket.u = saddr->u; + qapi_free_SocketAddress(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(),so free 'saddr' to avoid memory leak. Fixes: 72a8192e225c ("migration: convert migration 'uri' into 'MigrateAddress'") Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn> --- migration/migration.c | 1 + 1 file changed, 1 insertion(+)