Hi

On Thu, Nov 19, 2020 at 8:19 PM Daniele Buono <dbuono@linux.vnet.ibm.com>
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?
>
>
Any update on this patch?
thanks


-- 
Marc-André Lureau