Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 5/14/19 8:15 AM, Yury Kotov wrote:
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>> a bug with removing fd from fdset which id >= 2^32.
>> So, fix return types for these function.
>
> fd's cannot exceed 2^32. We should instead be fixing anything that uses
> int64_t with an fd to be properly limited to 32 bits.  That is, I think
> the real problem is in qapi/misc.json:
>
>  { 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> instead of 'fd':'int32'.

This is actually not related to the patch.  It doesn't touch
file-descriptors at all, only fdset IDs.

But let's discuss file descriptors briefly.

File descriptors are plain int.  There is no QAPI type corresponding to
plain int.

I guess plain int is 32 bits wide on all hosts we support.  Narrower int
(permitted by the standard) wouldn't fly with QEMU.  Wider int should,
and are theoretically possible.

I'm not sure we want to change the QAPI schema.

>                           For that matter, 'fdset-id' larger than 32
> bits is unlikely to be useful (there's no reason to have more fdsets
> than you can have possible fds to put in those sets).

Even if we had wider file descriptors: a billion fdsets should be enough
for anyone.

> NACK to this version, but a v2 that addresses the real problem is
> worthwhile.

What exactly is wrong with the patch?

It changes the return value of monitor_fdset_dup_fd_find_remove() and
monitor_fdset_dup_fd_find() from int to int64_t.  Both return an fdset
ID (a MonFdset member @id, of type int64_t) on success, -1 on error.
The change removes a truncation from int64_t to int in
monitor_fdset_dup_fd_find_remove(), and a widening from int to int64_t
in qemu_close().

I believe the patch is fine as is.  Another patch that changes fdset IDs
from int64_t to int32_t would also be fine, but it would Require
tracking down all the places to change.

[...]