| Message ID | 20231129204301.131228-1-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] migration: Plug memory leak with migration URIs | expand |
Het Gala <het.gala@nutanix.com> writes: > migrate_uri_parse() allocates memory to 'channel' if the user > opts for old syntax - uri, which is leaked because there is no > code for freeing 'channel'. > So, free channel to avoid memory leak in case where 'channels' > is empty and uri parsing is required. > > Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") > Signed-off-by: Het Gala <het.gala@nutanix.com> > Suggested-by: Markus Armbruster <armbru@redhat.com> Tested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote: > migrate_uri_parse() allocates memory to 'channel' if the user > opts for old syntax - uri, which is leaked because there is no > code for freeing 'channel'. > So, free channel to avoid memory leak in case where 'channels' > is empty and uri parsing is required. > > Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") > Signed-off-by: Het Gala <het.gala@nutanix.com> > Suggested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> > @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, > error_setg(errp, "Channel list has more than one entries"); > return; > } > - channel = channels->value; > + addr = channels->value->addr; > } else if (uri) { > /* caller uses the old URI syntax */ > if (!migrate_uri_parse(uri, &channel, errp)) { > return; > } > + addr = channel->addr; > } else { > error_setg(errp, "neither 'uri' or 'channels' argument are " > "specified in 'migrate-incoming' qmp command "); > return; > } > - addr = channel->addr; Why these "addr" lines need change? Won't that behave the same as before? Thanks,
Peter Xu <peterx@redhat.com> writes: > On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote: >> migrate_uri_parse() allocates memory to 'channel' if the user >> opts for old syntax - uri, which is leaked because there is no >> code for freeing 'channel'. >> So, free channel to avoid memory leak in case where 'channels' >> is empty and uri parsing is required. >> >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> Suggested-by: Markus Armbruster <armbru@redhat.com> > > Reviewed-by: Peter Xu <peterx@redhat.com> > >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, >> error_setg(errp, "Channel list has more than one entries"); >> return; >> } >> - channel = channels->value; >> + addr = channels->value->addr; >> } else if (uri) { >> /* caller uses the old URI syntax */ >> if (!migrate_uri_parse(uri, &channel, errp)) { >> return; >> } >> + addr = channel->addr; >> } else { >> error_setg(errp, "neither 'uri' or 'channels' argument are " >> "specified in 'migrate-incoming' qmp command "); >> return; >> } >> - addr = channel->addr; > > Why these "addr" lines need change? Won't that behave the same as before? In the first case, @channel is now null. If we left the assignment to @addr alone, it would crash. Clearer now?
On Thu, Nov 30, 2023 at 07:35:43PM +0100, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote: > >> migrate_uri_parse() allocates memory to 'channel' if the user > >> opts for old syntax - uri, which is leaked because there is no > >> code for freeing 'channel'. > >> So, free channel to avoid memory leak in case where 'channels' > >> is empty and uri parsing is required. > >> > >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") > >> Signed-off-by: Het Gala <het.gala@nutanix.com> > >> Suggested-by: Markus Armbruster <armbru@redhat.com> > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, > >> error_setg(errp, "Channel list has more than one entries"); > >> return; > >> } > >> - channel = channels->value; > >> + addr = channels->value->addr; > >> } else if (uri) { > >> /* caller uses the old URI syntax */ > >> if (!migrate_uri_parse(uri, &channel, errp)) { > >> return; > >> } > >> + addr = channel->addr; > >> } else { > >> error_setg(errp, "neither 'uri' or 'channels' argument are " > >> "specified in 'migrate-incoming' qmp command "); > >> return; > >> } > >> - addr = channel->addr; > > > > Why these "addr" lines need change? Won't that behave the same as before? > > In the first case, @channel is now null. If we left the assignment to > @addr alone, it would crash. Clearer now? Is it this one? if (uri && has_channels) { error_setg(errp, "'uri' and 'channels' arguments are mutually " "exclusive; exactly one of the two should be present in " "'migrate-incoming' qmp command "); return; } It returns already? Thanks,
Peter Xu <peterx@redhat.com> writes: > On Thu, Nov 30, 2023 at 07:35:43PM +0100, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote: >> >> migrate_uri_parse() allocates memory to 'channel' if the user >> >> opts for old syntax - uri, which is leaked because there is no >> >> code for freeing 'channel'. >> >> So, free channel to avoid memory leak in case where 'channels' >> >> is empty and uri parsing is required. >> >> >> >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") >> >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> > >> > Reviewed-by: Peter Xu <peterx@redhat.com> >> > >> >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, - MigrationChannel *channel = NULL; + g_autoptr(MigrationChannel) channel = NULL; MigrationAddress *addr = NULL; MigrationIncomingState *mis = migration_incoming_get_current(); /* * Having preliminary checks for uri and channel */ if (uri && has_channels) { error_setg(errp, "'uri' and 'channels' arguments are mutually " "exclusive; exactly one of the two should be present in " "'migrate-incoming' qmp command "); return; } else if (channels) { /* To verify that Migrate channel list has only item */ if (channels->next) { >> >> error_setg(errp, "Channel list has more than one entries"); >> >> return; >> >> } >> >> - channel = channels->value; >> >> + addr = channels->value->addr; >> >> } else if (uri) { >> >> /* caller uses the old URI syntax */ >> >> if (!migrate_uri_parse(uri, &channel, errp)) { >> >> return; >> >> } >> >> + addr = channel->addr; >> >> } else { >> >> error_setg(errp, "neither 'uri' or 'channels' argument are " >> >> "specified in 'migrate-incoming' qmp command "); >> >> return; >> >> } >> >> - addr = channel->addr; >> > >> > Why these "addr" lines need change? Won't that behave the same as before? >> >> In the first case, @channel is now null. If we left the assignment to >> @addr alone, it would crash. Clearer now? > > Is it this one? > > if (uri && has_channels) { > error_setg(errp, "'uri' and 'channels' arguments are mutually " > "exclusive; exactly one of the two should be present in " > "'migrate-incoming' qmp command "); > return; > } > > It returns already? I meant the first visible case, i.e. if (channels). Sorry for being less than clear! The problem is to free the result of migrate_uri_parse(). The patch's solution is to use @channel *only* for holding that result, so it can be g_autoptr: drop channel = channels->value from the if (channels) conditional. Since this breaks addr = channel->addr, we move that assignment into the conditionals that reach it, which lets us unbreak it the if (channels) one.
On Fri, Dec 01, 2023 at 07:19:45AM +0100, Markus Armbruster wrote: > I meant the first visible case, i.e. if (channels). Sorry for being > less than clear! > > The problem is to free the result of migrate_uri_parse(). > > The patch's solution is to use @channel *only* for holding that result, > so it can be g_autoptr: drop channel = channels->value from the if > (channels) conditional. > > Since this breaks addr = channel->addr, we move that assignment into the > conditionals that reach it, which lets us unbreak it the if (channels) > one. My bad! It also proved that my R-b was bold. :( Thanks, Markus. Since Juan's away, I'll prepare a pull.
diff --git a/migration/migration.c b/migration/migration.c index 28a34c9068..34340f3440 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -515,7 +515,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, MigrationChannelList *channels, Error **errp) { - MigrationChannel *channel = NULL; + g_autoptr(MigrationChannel) channel = NULL; MigrationAddress *addr = NULL; MigrationIncomingState *mis = migration_incoming_get_current(); @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, error_setg(errp, "Channel list has more than one entries"); return; } - channel = channels->value; + addr = channels->value->addr; } else if (uri) { /* caller uses the old URI syntax */ if (!migrate_uri_parse(uri, &channel, errp)) { return; } + addr = channel->addr; } else { error_setg(errp, "neither 'uri' or 'channels' argument are " "specified in 'migrate-incoming' qmp command "); return; } - addr = channel->addr; /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) { @@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels, bool resume_requested; Error *local_err = NULL; MigrationState *s = migrate_get_current(); - MigrationChannel *channel = NULL; + g_autoptr(MigrationChannel) channel = NULL; MigrationAddress *addr = NULL; /* @@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels, error_setg(errp, "Channel list has more than one entries"); return; } - channel = channels->value; + addr = channels->value->addr; } else if (uri) { /* caller uses the old URI syntax */ if (!migrate_uri_parse(uri, &channel, errp)) { return; } + addr = channel->addr; } else { error_setg(errp, "neither 'uri' or 'channels' argument are " "specified in 'migrate' qmp command "); return; } - addr = channel->addr; /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) {
migrate_uri_parse() allocates memory to 'channel' if the user opts for old syntax - uri, which is leaked because there is no code for freeing 'channel'. So, free channel to avoid memory leak in case where 'channels' is empty and uri parsing is required. Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") Signed-off-by: Het Gala <het.gala@nutanix.com> Suggested-by: Markus Armbruster <armbru@redhat.com> --- migration/migration.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)