Re: [PULL 27/34] migration/multifd: Move nocomp code into multifd-nocomp.c

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 4 Sept 2024 at 13:48, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> In preparation for adding new payload types to multifd, move most of
>> the no-compression code into multifd-nocomp.c. Let's try to keep a
>> semblance of layering by not mixing general multifd control flow with
>> the details of transmitting pages of ram.
>>
>> There are still some pieces leftover, namely the p->normal, p->zero,
>> etc variables that we use for zero page tracking and the packet
>> allocation which is heavily dependent on the ram code.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> I know Coverity has only flagged this up because the
> code has moved, but it seems like a good place to ask
> the question:
>
>> +void multifd_ram_fill_packet(MultiFDSendParams *p)
>> +{
>> +    MultiFDPacket_t *packet = p->packet;
>> +    MultiFDPages_t *pages = &p->data->u.ram;
>> +    uint32_t zero_num = pages->num - pages->normal_num;
>> +
>> +    packet->pages_alloc = cpu_to_be32(multifd_ram_page_count());
>> +    packet->normal_pages = cpu_to_be32(pages->normal_num);
>> +    packet->zero_pages = cpu_to_be32(zero_num);
>> +
>> +    if (pages->block) {
>> +        strncpy(packet->ramblock, pages->block->idstr, 256);
>
> Coverity points out that when we fill in the RAMBlock::idstr
> here, if packet->ramblock is not NUL terminated then we won't
> NUL-terminate idstr either (CID 1560071).
>
> Is this really what is intended?

This is probably an oversight, although the migration destination
truncates it before reading:

    /* make sure that ramblock is 0 terminated */
    packet->ramblock[255] = 0;
    p->block = qemu_ram_block_by_name(packet->ramblock);

If we ever start reading packet->ramblock on the source side in the
future, then there might be a problem.

>
> Perhaps
>          pstrncpy(packet->ramblock, sizeof(packet->ramblock),
>                   pages->block->idstr);
>
> would be better?

Yep, thanks. I'll send a patch.

>
> (pstrncpy will always NUL-terminate, and won't pointlessly
> zero-fill the space after the string in the destination.)
>
>> +    }
>> +
>> +    for (int i = 0; i < pages->num; i++) {
>> +        /* there are architectures where ram_addr_t is 32 bit */
>> +        uint64_t temp = pages->offset[i];
>> +
>> +        packet->offset[i] = cpu_to_be64(temp);
>> +    }
>> +
>> +    trace_multifd_send_ram_fill(p->id, pages->normal_num,
>> +                                zero_num);
>> +}
>
> thanks
> -- PMM