Peter Maydell <[email protected]> wrote:
> On Thu, 2 Nov 2023 at 11:46, Juan Quintela <[email protected]> wrote:
>>
>> From: Het Gala <[email protected]>
>>
>> Integrate MigrateChannelList with all transport backends
>> (socket, exec and rdma) for both src and dest migration
>> endpoints for qmp migration.
>>
>> For current series, limit the size of MigrateChannelList
>> to single element (single interface) as runtime check.
>>
>> Suggested-by: Aravind Retnakaran <[email protected]>
>> Signed-off-by: Het Gala <[email protected]>
>> Signed-off-by: Fabiano Rosas <[email protected]>
>> Reviewed-by: Juan Quintela <[email protected]>
>> Signed-off-by: Juan Quintela <[email protected]>
>> Message-ID: <[email protected]>
>
> Hi; after this migration pull there seem to be a lot of
> new Coverity issues in migration code. Could somebody
> take a look at them, please?
Hi
I received this, looking into it.
Later, Juan.
>
> Here's one in particular (CID 1523826, 1523828):
>
>> @@ -1927,35 +1933,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>> bool resume_requested;
>> Error *local_err = NULL;
>> MigrationState *s = migrate_get_current();
>> - g_autoptr(MigrationAddress) channel = NULL;
>> + MigrationChannel *channel = NULL;
>> + MigrationAddress *addr = NULL;
>
> 'channel' in this function used to be auto-freed, but now it is not...
>
>>
>> /*
>> * Having preliminary checks for uri and channel
>> */
>> - if (has_channels) {
>> - error_setg(errp, "'channels' argument should not be set yet.");
>> - return;
>> - }
>> -
>> if (uri && has_channels) {
>> error_setg(errp, "'uri' and 'channels' arguments are mutually "
>> "exclusive; exactly one of the two should be present in "
>> "'migrate' qmp command ");
>> return;
>> - }
>> -
>> - if (!uri && !has_channels) {
>> + } 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;
>> + } else if (uri) {
>> + /* caller uses the old URI syntax */
>> + if (!migrate_uri_parse(uri, &channel, errp)) {
>> + return;
>> + }
>
> ...and here migrate_uri_parse() allocates memory which is
> returned into 'channel'...
>
>> + } else {
>> error_setg(errp, "neither 'uri' or 'channels' argument are "
>> "specified in 'migrate' qmp command ");
>> return;
>> }
>> -
>> - if (!migrate_uri_parse(uri, &channel, errp)) {
>> - return;
>> - }
>> + addr = channel->addr;
>>
>> /* transport mechanism not suitable for migration? */
>> - if (!migration_channels_and_transport_compatible(channel, errp)) {
>> + if (!migration_channels_and_transport_compatible(addr, errp)) {
>> return;
>> }
>>
>> @@ -1972,8 +1981,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>> }
>> }
>>
>> - if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> - SocketAddress *saddr = &channel->u.socket;
>> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> + SocketAddress *saddr = &addr->u.socket;
>> if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> @@ -1982,13 +1991,13 @@ void qmp_migrate(const char *uri, bool has_channels,
>> fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>> }
>> #ifdef CONFIG_RDMA
>> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> - rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
>> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> + rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
>> #endif
>> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> - exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
>> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
>> - file_start_outgoing_migration(s, &channel->u.file, &local_err);
>> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> + exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
>> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
>> + file_start_outgoing_migration(s, &addr->u.file, &local_err);
>> } else {
>> error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>> "a valid migration protocol");
>
> ...which is leaked because we don't add any code for freeing
> channel to compensate for it no longer being autofreed.
>
> thanks
> -- PMM
