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?
