Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Philippe Mathieu-Daudé]

On 11/19/20 5:16 PM, Daniele Buono wrote:
> Hi Philippe,
> 
> On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote:
>> On 11/5/20 11:18 PM, Daniele Buono wrote:
>>> The UASStatus data structure has a variable sized field inside of
>>> type uas_iu,
>>> that however is not placed at the end of the data structure.
>>>
>>> This placement triggers a warning with clang 11, and while not a bug
>>> right now,
>>> (the status is never a uas_iu_command, which is the variable-sized
>>> case),
>>> it could become one in the future.
>>
>> The problem is uas_iu_command::add_cdb, indeed.
>>
>>>
>>> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with
>>> variable sized type 'uas_iu' not at the end of a struct or class is a
>>> GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>>
>> If possible remove the "../qemu-base/" as it does not provide
>> any useful information.
>>
> Sure, will do at the next cycle
>>>      uas_iu                    status;
>>>                                ^
>>> 1 error generated.
>>>
>>> Fix this by moving uas_iu at the end of the struct
>>
>> Your patch silents the warning, but the problem is the same.
>> It would be safer/cleaner to make 'status' a pointer on the heap IMO.
> 
> I'm thinking of moving 'status' in a pointer with the following code
> changes:
> 
> UASStatus is allocated in `usb_uas_alloc_status`, which currently does
> not take a type or size for the union field. I'm thinking of adding
> requested size for the status, like this:
> 
> static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id,
> uint16_t tag, size_t size);
> 
> and the common call would be
> usb_uas_alloc_status([...],sizeof(uas_iu));
> 
> Also we'd need a double free when the object is freed. Right now
> it's handled in the code when the object is not used anymore with a
> `g_free(st);`.
> I'd have to replace it with
> `g_free(st->status); g_free(st);`. Would you suggest doing it place
> or by adding a usb_uas_dealloc_status() function?
> 
> ---
> 
> However, I am confused by the use of that variable-lenght field.
> I'm looking at what seems the only place where a command is
> parsed, in `usb_uas_handle_data`.
> 
> uas_iu iu;
> [...]
>     switch (p->ep->nr) {
>     case UAS_PIPE_ID_COMMAND:
>         length = MIN(sizeof(iu), p->iov.size);
>         usb_packet_copy(p, &iu, length);
>         [...]
>         break;
> [...]
> 
> It would seem that the copy is limited to at most sizeof(uas_iu),
> so even if we had anything in add_cdb[], that wouldn't be copied
> here?
> 
> Is this intended?

Gerd, do you know?