Hey,
I was looking at the code in profiles/audio/avdtp.c surrounding those
static analyser warnings, and couldn't understand how the seid arrays
were constructed.
There's similar code in android/ which might also need fixing.
I could only find the code that assigned ".first_seid", nothing about
how the rest of the structure is allocated and assigned.
Cheers
PS: Please CC: on the answer, as I'm not subscribed to the list
Error: ARRAY_VS_SINGLETON (CWE-119): [#def29] [important]
bluez-5.75/profiles/audio/avdtp.c:1675:2: address_of: Taking address with "&start->first_seid" yields a singleton pointer.
bluez-5.75/profiles/audio/avdtp.c:1675:2: assign: Assigning: "seid" = "&start->first_seid".
bluez-5.75/profiles/audio/avdtp.c:1679:25: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
# 1677| int i;
# 1678|
# 1679|-> for (i = 0; i < count; i++, seid++) {
# 1680| if (seid->seid == id) {
# 1681| req->collided = TRUE;
Error: ARRAY_VS_SINGLETON (CWE-119): [#def30] [important]
bluez-5.75/profiles/audio/avdtp.c:1690:2: address_of: Taking address with "&suspend->first_seid" yields a singleton pointer.
bluez-5.75/profiles/audio/avdtp.c:1690:2: assign: Assigning: "seid" = "&suspend->first_seid".
bluez-5.75/profiles/audio/avdtp.c:1694:25: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
# 1692| int i;
# 1693|
# 1694|-> for (i = 0; i < count; i++, seid++) {
# 1695| if (seid->seid == id) {
# 1696| req->collided = TRUE;
Error: ARRAY_VS_SINGLETON (CWE-119): [#def31] [important]
bluez-5.75/profiles/audio/avdtp.c:1799:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
bluez-5.75/profiles/audio/avdtp.c:1799:2: assign: Assigning: "seid" = "&req->first_seid".
bluez-5.75/profiles/audio/avdtp.c:1801:30: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
# 1799| seid = &req->first_seid;
# 1800|
# 1801|-> for (i = 0; i < seid_count; i++, seid++) {
# 1802| failed_seid = seid->seid;
# 1803|
Error: ARRAY_VS_SINGLETON (CWE-119): [#def32] [important]
bluez-5.75/profiles/audio/avdtp.c:1912:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
bluez-5.75/profiles/audio/avdtp.c:1912:2: assign: Assigning: "seid" = "&req->first_seid".
bluez-5.75/profiles/audio/avdtp.c:1914:30: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
# 1912| seid = &req->first_seid;
# 1913|
# 1914|-> for (i = 0; i < seid_count; i++, seid++) {
# 1915| failed_seid = seid->seid;
# 1916|
Hi,
to, 2024-05-16 kello 11:13 +0200, Bastien Nocera kirjoitti:
> I was looking at the code in profiles/audio/avdtp.c surrounding those
> static analyser warnings, and couldn't understand how the seid arrays
> were constructed.
>
> There's similar code in android/ which might also need fixing.
>
> I could only find the code that assigned ".first_seid", nothing about
> how the rest of the structure is allocated and assigned.
These structs are from AVDTP spec, see eg. §8.13 for Start Stream
Command <-> struct start_req.
IIUC, they're actually arrays of struct seid, but the first element is
defined as a separate field. I guess the static checker chokes on that,
and not sure right now if this is even strictly allowed in C.
The structures are allocated in send_request() for the outgoing
messages and the bounds checking is via req->data_size. For incoming
messages they're raw message data from the remote device.
> Cheers
>
> PS: Please CC: on the answer, as I'm not subscribed to the list
>
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def29] [important]
> bluez-5.75/profiles/audio/avdtp.c:1675:2: address_of: Taking address with "&start->first_seid" yields a singleton pointer.
> bluez-5.75/profiles/audio/avdtp.c:1675:2: assign: Assigning: "seid" = "&start->first_seid".
> bluez-5.75/profiles/audio/avdtp.c:1679:25: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
> # 1677| int i;
> # 1678|
> # 1679|-> for (i = 0; i < count; i++, seid++) {
> # 1680| if (seid->seid == id) {
> # 1681| req->collided = TRUE;
>
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def30] [important]
> bluez-5.75/profiles/audio/avdtp.c:1690:2: address_of: Taking address with "&suspend->first_seid" yields a singleton pointer.
> bluez-5.75/profiles/audio/avdtp.c:1690:2: assign: Assigning: "seid" = "&suspend->first_seid".
> bluez-5.75/profiles/audio/avdtp.c:1694:25: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
> # 1692| int i;
> # 1693|
> # 1694|-> for (i = 0; i < count; i++, seid++) {
> # 1695| if (seid->seid == id) {
> # 1696| req->collided = TRUE;
>
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def31] [important]
> bluez-5.75/profiles/audio/avdtp.c:1799:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
> bluez-5.75/profiles/audio/avdtp.c:1799:2: assign: Assigning: "seid" = "&req->first_seid".
> bluez-5.75/profiles/audio/avdtp.c:1801:30: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
> # 1799| seid = &req->first_seid;
> # 1800|
> # 1801|-> for (i = 0; i < seid_count; i++, seid++) {
> # 1802| failed_seid = seid->seid;
> # 1803|
>
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def32] [important]
> bluez-5.75/profiles/audio/avdtp.c:1912:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
> bluez-5.75/profiles/audio/avdtp.c:1912:2: assign: Assigning: "seid" = "&req->first_seid".
> bluez-5.75/profiles/audio/avdtp.c:1914:30: ptr_arith: Using "seid" as an array. This might corrupt or misinterpret adjacent memory locations.
> # 1912| seid = &req->first_seid;
> # 1913|
> # 1914|-> for (i = 0; i < seid_count; i++, seid++) {
> # 1915| failed_seid = seid->seid;
> # 1916|
--
Pauli Virtanen
On Thu, 2024-05-16 at 19:00 +0300, Pauli Virtanen wrote:
> Hi,
>
> to, 2024-05-16 kello 11:13 +0200, Bastien Nocera kirjoitti:
> > I was looking at the code in profiles/audio/avdtp.c surrounding
> > those
> > static analyser warnings, and couldn't understand how the seid
> > arrays
> > were constructed.
> >
> > There's similar code in android/ which might also need fixing.
> >
> > I could only find the code that assigned ".first_seid", nothing
> > about
> > how the rest of the structure is allocated and assigned.
>
> These structs are from AVDTP spec, see eg. §8.13 for Start Stream
> Command <-> struct start_req.
>
> IIUC, they're actually arrays of struct seid, but the first element
> is
> defined as a separate field. I guess the static checker chokes on
> that,
> and not sure right now if this is even strictly allowed in C.
>
> The structures are allocated in send_request() for the outgoing
> messages and the bounds checking is via req->data_size. For incoming
> messages they're raw message data from the remote device.
Thanks Pauli for the explanation.
The static analysers didn't like that construct, and I've attempted to
fix it in:
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#u
If that doesn't pass muster, we can also do a simple "#define start_seq
suspend_seq" and have a single actual struct type for both cases.
Cheers
>
>
> > Cheers
> >
> > PS: Please CC: on the answer, as I'm not subscribed to the list
> >
> > Error: ARRAY_VS_SINGLETON (CWE-119): [#def29] [important]
> > bluez-5.75/profiles/audio/avdtp.c:1675:2: address_of: Taking
> > address with "&start->first_seid" yields a singleton pointer.
> > bluez-5.75/profiles/audio/avdtp.c:1675:2: assign: Assigning: "seid"
> > = "&start->first_seid".
> > bluez-5.75/profiles/audio/avdtp.c:1679:25: ptr_arith: Using "seid"
> > as an array. This might corrupt or misinterpret adjacent memory
> > locations.
> > # 1677| int i;
> > # 1678|
> > # 1679|-> for (i = 0; i < count; i++, seid++) {
> > # 1680| if (seid->seid == id) {
> > # 1681| req->collided = TRUE;
> >
> > Error: ARRAY_VS_SINGLETON (CWE-119): [#def30] [important]
> > bluez-5.75/profiles/audio/avdtp.c:1690:2: address_of: Taking
> > address with "&suspend->first_seid" yields a singleton pointer.
> > bluez-5.75/profiles/audio/avdtp.c:1690:2: assign: Assigning: "seid"
> > = "&suspend->first_seid".
> > bluez-5.75/profiles/audio/avdtp.c:1694:25: ptr_arith: Using "seid"
> > as an array. This might corrupt or misinterpret adjacent memory
> > locations.
> > # 1692| int i;
> > # 1693|
> > # 1694|-> for (i = 0; i < count; i++, seid++) {
> > # 1695| if (seid->seid == id) {
> > # 1696| req->collided = TRUE;
> >
> > Error: ARRAY_VS_SINGLETON (CWE-119): [#def31] [important]
> > bluez-5.75/profiles/audio/avdtp.c:1799:2: address_of: Taking
> > address with "&req->first_seid" yields a singleton pointer.
> > bluez-5.75/profiles/audio/avdtp.c:1799:2: assign: Assigning: "seid"
> > = "&req->first_seid".
> > bluez-5.75/profiles/audio/avdtp.c:1801:30: ptr_arith: Using "seid"
> > as an array. This might corrupt or misinterpret adjacent memory
> > locations.
> > # 1799| seid = &req->first_seid;
> > # 1800|
> > # 1801|-> for (i = 0; i < seid_count; i++, seid++) {
> > # 1802| failed_seid = seid->seid;
> > # 1803|
> >
> > Error: ARRAY_VS_SINGLETON (CWE-119): [#def32] [important]
> > bluez-5.75/profiles/audio/avdtp.c:1912:2: address_of: Taking
> > address with "&req->first_seid" yields a singleton pointer.
> > bluez-5.75/profiles/audio/avdtp.c:1912:2: assign: Assigning: "seid"
> > = "&req->first_seid".
> > bluez-5.75/profiles/audio/avdtp.c:1914:30: ptr_arith: Using "seid"
> > as an array. This might corrupt or misinterpret adjacent memory
> > locations.
> > # 1912| seid = &req->first_seid;
> > # 1913|
> > # 1914|-> for (i = 0; i < seid_count; i++, seid++) {
> > # 1915| failed_seid = seid->seid;
> > # 1916|
>