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.
../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]
uas_iu status;
^
1 error generated.
Fix this by moving uas_iu at the end of the struct
Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
hw/usb/dev-uas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index cec071d96c..5ef3f4fec9 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -154,9 +154,9 @@ struct UASRequest {
struct UASStatus {
uint32_t stream;
- uas_iu status;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
+ uas_iu status;
};
/* --------------------------------------------------------------------- */
--
2.17.1
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.
> 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.
>
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
> hw/usb/dev-uas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index cec071d96c..5ef3f4fec9 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -154,9 +154,9 @@ struct UASRequest {
>
> struct UASStatus {
> uint32_t stream;
> - uas_iu status;
> uint32_t length;
> QTAILQ_ENTRY(UASStatus) next;
> + uas_iu status;
> };
>
> /* --------------------------------------------------------------------- */
>
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?
>
>>
>> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>> ---
>> hw/usb/dev-uas.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
>> index cec071d96c..5ef3f4fec9 100644
>> --- a/hw/usb/dev-uas.c
>> +++ b/hw/usb/dev-uas.c
>> @@ -154,9 +154,9 @@ struct UASRequest {
>>
>> struct UASStatus {
>> uint32_t stream;
>> - uas_iu status;
>> uint32_t length;
>> QTAILQ_ENTRY(UASStatus) next;
>> + uas_iu status;
>> };
>>
>> /* --------------------------------------------------------------------- */
>>
>
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
On 1/14/2021 3:17 AM, Marc-André Lureau wrote:
>
> Any update on this patch?
> thanks
Hi,
I had been waiting for a feedback on my previous message.
I don't know the UAS subsystem that well, but can't find where the
variable-sized field that is causing the issue is actually used.
If it helps, I can send an RFC for converting
struct UASStatus {
uint32_t stream;
uas_iu status;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
};
to
struct UASStatus {
uint32_t stream;
uas_iu *status;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
};
And discuss it at that point.
Thanks,
Daniele
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?
Hi, > > 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? Don't remember, it's been a few years ago ... Given that neither add_cdb nor add_cdb_length fields are checked anywhere in the code it is probably save to just comment out the add_cdb field. Should we ever need to look at add_cdb at some point in the future we can figure some better way deal with it, in the simplest case just give it a fixed size based on the needs. take care, Gerd
© 2016 - 2026 Red Hat, Inc.