Bluez has fully broken profiles/audio/a2dp-codecs.h file for big endian

Bluez has fully broken profiles/audio/a2dp-codecs.h file for big endian

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

Hello,

I spotted that bluez has file profiles/audio/a2dp-codecs.h in its git
repository with some #if __BYTE_ORDER == checks, including #ifdef for
big endian systems.

But this file is fully broken for big endian systems. For example, there
is following structure which is same for both little and big endian
systems:

  typedef struct {
  	uint32_t vendor_id;
  	uint16_t codec_id;
  } __attribute__ ((packed)) a2dp_vendor_codec_t;

Which is valid only for little endian systems, as those definitions are
bound to little endian ordering -- not to host ordering.

Next there is a2dp_mpeg_t. It has different definition for little endian
and big endian systems.

little endian:

  typedef struct {
  	uint8_t channel_mode:4;
  	uint8_t crc:1;
  	uint8_t layer:3;
  	uint8_t frequency:6;
  	uint8_t mpf:1;
  	uint8_t rfa:1;
  	uint16_t bitrate;
  } __attribute__ ((packed)) a2dp_mpeg_t;

big endian:

  typedef struct {
  	uint8_t layer:3;
  	uint8_t crc:1;
  	uint8_t channel_mode:4;
  	uint8_t rfa:1;
  	uint8_t mpf:1;
  	uint8_t frequency:6;
  	uint16_t bitrate;
  } __attribute__ ((packed)) a2dp_mpeg_t;

But as you can see, bitrate on big endian definition is incorrect. It
is still in little endian. Proper way would be to define bitrate as
"uint8_t bitrate[2];" or as "uint8_t bitrate_lo; uint8_t bitrate_hi".

So basically this file a2dp-codecs.h is not compatible with big endian
systems, even there are "#elif __BYTE_ORDER == __BIG_ENDIAN" branches.

If you are not going to support big endian systems, I suggest you to
drop all those "#elif __BYTE_ORDER == __BIG_ENDIAN" parts as people
think that existence of these macros means that code is supported on big
endian systems.

It is better throw an error that big endian is broken as trying to
produce non-working binaries.

Moreover, some people started copying this a2dp-codecs.h file to other
projects which just leads to copy+paste of broken code.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Bluez has fully broken profiles/audio/a2dp-codecs.h file for big endian

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

On Tuesday 18 December 2018 17:50:52 Luiz Augusto von Dentz wrote:
> On Tue, 18 Dec 2018, 11:54 Pali Rohár <pali.rohar@gmail.com wrote:
> 
> > Hello,
> >
> > I spotted that bluez has file profiles/audio/a2dp-codecs.h in its git
> > repository with some #if __BYTE_ORDER == checks, including #ifdef for
> > big endian systems.
> >
> > But this file is fully broken for big endian systems. For example, there
> > is following structure which is same for both little and big endian
> > systems:
> >
> >   typedef struct {
> >         uint32_t vendor_id;
> >         uint16_t codec_id;
> >   } __attribute__ ((packed)) a2dp_vendor_codec_t;
> >
> 
> Byte order is not the same as packet order, in this case there is no
> difference between big or little endian because there is no use of bit
> fields, the packet ordering of the fields is the same regardless of the
> byte order meaning that vendor_id is transmitted before codec_id since the
> spec actually defines them in this way and it is wrong to alter this order.
> You can check how btmon decodes this and you would see it only decodes
> individual fields to host not the entire struct representing the packet.
> 
> Which is valid only for little endian systems, as those definitions are
> > bound to little endian ordering -- not to host ordering.
> >
> 
> The spec defines the packets in little endian not in host endian.

But in code, you work with host endian. Therefore uint32_t is in host
endian too. Data are transferred in little endian (as you wrote), so you
need to do some conversion from little endian to host endian.

Or you new gcc6 feature

__attribute__((packed, scalar_storage_order("little-endian")))

to tell that structure field is in little endian and not in host endian.

> Next there is a2dp_mpeg_t. It has different definition for little endian
> > and big endian systems.
> >
> > little endian:
> >
> >   typedef struct {
> >         uint8_t channel_mode:4;
> >         uint8_t crc:1;
> >         uint8_t layer:3;
> >         uint8_t frequency:6;
> >         uint8_t mpf:1;
> >         uint8_t rfa:1;
> >         uint16_t bitrate;
> >   } __attribute__ ((packed)) a2dp_mpeg_t;
> >
> > big endian:
> >
> >   typedef struct {
> >         uint8_t layer:3;
> >         uint8_t crc:1;
> >         uint8_t channel_mode:4;
> >         uint8_t rfa:1;
> >         uint8_t mpf:1;
> >         uint8_t frequency:6;
> >         uint16_t bitrate;
> >   } __attribute__ ((packed)) a2dp_mpeg_t;
> >
> > But as you can see, bitrate on big endian definition is incorrect. It
> > is still in little endian. Proper way would be to define bitrate as
> > "uint8_t bitrate[2];" or as "uint8_t bitrate_lo; uint8_t bitrate_hi".
> >
> 
> Obviously one need to convert the value when receiving, or perhaps you are
> saying it makes no sense to use bit fields in that case? Anyway when
> accessing this the user must convert to host order of he intends to use the
> value.

Yes, that is the point, user needs to convert values. But they are
marked with types uint32_t and uint16_t, which are types normally used
for host endian. This just confuse people who copy+paste this file to
other projects.

So if correct way is to always convert these values, what about creating
some typedef uint16_t le_uint16_t or add comments that those values are
in little endian and not in host endian?

Or usage of __attribute__((packed, scalar_storage_order("little-endian")))?

> So basically this file a2dp-codecs.h is not compatible with big endian
> > systems, even there are "#elif __BYTE_ORDER == __BIG_ENDIAN" branches.
> >
> > If you are not going to support big endian systems, I suggest you to
> > drop all those "#elif __BYTE_ORDER == __BIG_ENDIAN" parts as people
> > think that existence of these macros means that code is supported on big
> > endian systems.
> >
> > It is better throw an error that big endian is broken as trying to
> > produce non-working binaries.
> >
> > Moreover, some people started copying this a2dp-codecs.h file to other
> > projects which just leads to copy+paste of broken code.
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> >

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Bluez has fully broken profiles/audio/a2dp-codecs.h file for big endian

[Luiz Augusto von Dentz]

Hi Pali,

On Tue, Dec 18, 2018 at 6:07 PM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Tuesday 18 December 2018 17:50:52 Luiz Augusto von Dentz wrote:
> > On Tue, 18 Dec 2018, 11:54 Pali Rohár <pali.rohar@gmail.com wrote:
> >
> > > Hello,
> > >
> > > I spotted that bluez has file profiles/audio/a2dp-codecs.h in its git
> > > repository with some #if __BYTE_ORDER == checks, including #ifdef for
> > > big endian systems.
> > >
> > > But this file is fully broken for big endian systems. For example, there
> > > is following structure which is same for both little and big endian
> > > systems:
> > >
> > >   typedef struct {
> > >         uint32_t vendor_id;
> > >         uint16_t codec_id;
> > >   } __attribute__ ((packed)) a2dp_vendor_codec_t;
> > >
> >
> > Byte order is not the same as packet order, in this case there is no
> > difference between big or little endian because there is no use of bit
> > fields, the packet ordering of the fields is the same regardless of the
> > byte order meaning that vendor_id is transmitted before codec_id since the
> > spec actually defines them in this way and it is wrong to alter this order.
> > You can check how btmon decodes this and you would see it only decodes
> > individual fields to host not the entire struct representing the packet.
> >
> > Which is valid only for little endian systems, as those definitions are
> > > bound to little endian ordering -- not to host ordering.
> > >
> >
> > The spec defines the packets in little endian not in host endian.
>
> But in code, you work with host endian. Therefore uint32_t is in host
> endian too. Data are transferred in little endian (as you wrote), so you
> need to do some conversion from little endian to host endian.
>
> Or you new gcc6 feature
>
> __attribute__((packed, scalar_storage_order("little-endian")))
>
> to tell that structure field is in little endian and not in host endian.
>
> > Next there is a2dp_mpeg_t. It has different definition for little endian
> > > and big endian systems.
> > >
> > > little endian:
> > >
> > >   typedef struct {
> > >         uint8_t channel_mode:4;
> > >         uint8_t crc:1;
> > >         uint8_t layer:3;
> > >         uint8_t frequency:6;
> > >         uint8_t mpf:1;
> > >         uint8_t rfa:1;
> > >         uint16_t bitrate;
> > >   } __attribute__ ((packed)) a2dp_mpeg_t;
> > >
> > > big endian:
> > >
> > >   typedef struct {
> > >         uint8_t layer:3;
> > >         uint8_t crc:1;
> > >         uint8_t channel_mode:4;
> > >         uint8_t rfa:1;
> > >         uint8_t mpf:1;
> > >         uint8_t frequency:6;
> > >         uint16_t bitrate;
> > >   } __attribute__ ((packed)) a2dp_mpeg_t;
> > >
> > > But as you can see, bitrate on big endian definition is incorrect. It
> > > is still in little endian. Proper way would be to define bitrate as
> > > "uint8_t bitrate[2];" or as "uint8_t bitrate_lo; uint8_t bitrate_hi".
> > >
> >
> > Obviously one need to convert the value when receiving, or perhaps you are
> > saying it makes no sense to use bit fields in that case? Anyway when
> > accessing this the user must convert to host order of he intends to use the
> > value.
>
> Yes, that is the point, user needs to convert values. But they are
> marked with types uint32_t and uint16_t, which are types normally used
> for host endian. This just confuse people who copy+paste this file to
> other projects.

packed struct are normally used just for casting using the received
buffer, so they should never be used as in host order but perhaps that
is not clearer for everyone.

> So if correct way is to always convert these values, what about creating
> some typedef uint16_t le_uint16_t or add comments that those values are
> in little endian and not in host endian?
>
> Or usage of __attribute__((packed, scalar_storage_order("little-endian")))?

Never used the scalar_storage_order, perhaps it would be a nice
addtion here, not sure if that does the convertion implicitly?

> > So basically this file a2dp-codecs.h is not compatible with big endian
> > > systems, even there are "#elif __BYTE_ORDER == __BIG_ENDIAN" branches.
> > >
> > > If you are not going to support big endian systems, I suggest you to
> > > drop all those "#elif __BYTE_ORDER == __BIG_ENDIAN" parts as people
> > > think that existence of these macros means that code is supported on big
> > > endian systems.

It is indeed broken for big endian if no convertion is performed, the
use o bitfield actually makes a mess in the structs so perhaps it is
better to use just full bytes and perhaps have macros for accesing the
bit fields, anyway we don't use them anymore in the daemon itself so
it is more of an issue in the likes of PulseAudio, neverthless we
should probably have proper definitions anyway in case anyone use them
as a reference.

> > > It is better throw an error that big endian is broken as trying to
> > > produce non-working binaries.
> > >
> > > Moreover, some people started copying this a2dp-codecs.h file to other
> > > projects which just leads to copy+paste of broken code.
> > >
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com
> > >
>
> --
> Pali Rohár
> pali.rohar@gmail.com



-- 
Luiz Augusto von Dentz

Re: Bluez has fully broken profiles/audio/a2dp-codecs.h file for big endian

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 6986 bytes --]

On Wednesday 19 December 2018 10:20:22 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Tue, Dec 18, 2018 at 6:07 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Tuesday 18 December 2018 17:50:52 Luiz Augusto von Dentz wrote:
> > > On Tue, 18 Dec 2018, 11:54 Pali Rohár <pali.rohar@gmail.com wrote:
> > >
> > > > Hello,
> > > >
> > > > I spotted that bluez has file profiles/audio/a2dp-codecs.h in its git
> > > > repository with some #if __BYTE_ORDER == checks, including #ifdef for
> > > > big endian systems.
> > > >
> > > > But this file is fully broken for big endian systems. For example, there
> > > > is following structure which is same for both little and big endian
> > > > systems:
> > > >
> > > >   typedef struct {
> > > >         uint32_t vendor_id;
> > > >         uint16_t codec_id;
> > > >   } __attribute__ ((packed)) a2dp_vendor_codec_t;
> > > >
> > >
> > > Byte order is not the same as packet order, in this case there is no
> > > difference between big or little endian because there is no use of bit
> > > fields, the packet ordering of the fields is the same regardless of the
> > > byte order meaning that vendor_id is transmitted before codec_id since the
> > > spec actually defines them in this way and it is wrong to alter this order.
> > > You can check how btmon decodes this and you would see it only decodes
> > > individual fields to host not the entire struct representing the packet.
> > >
> > > Which is valid only for little endian systems, as those definitions are
> > > > bound to little endian ordering -- not to host ordering.
> > > >
> > >
> > > The spec defines the packets in little endian not in host endian.
> >
> > But in code, you work with host endian. Therefore uint32_t is in host
> > endian too. Data are transferred in little endian (as you wrote), so you
> > need to do some conversion from little endian to host endian.
> >
> > Or you new gcc6 feature
> >
> > __attribute__((packed, scalar_storage_order("little-endian")))
> >
> > to tell that structure field is in little endian and not in host endian.
> >
> > > Next there is a2dp_mpeg_t. It has different definition for little endian
> > > > and big endian systems.
> > > >
> > > > little endian:
> > > >
> > > >   typedef struct {
> > > >         uint8_t channel_mode:4;
> > > >         uint8_t crc:1;
> > > >         uint8_t layer:3;
> > > >         uint8_t frequency:6;
> > > >         uint8_t mpf:1;
> > > >         uint8_t rfa:1;
> > > >         uint16_t bitrate;
> > > >   } __attribute__ ((packed)) a2dp_mpeg_t;
> > > >
> > > > big endian:
> > > >
> > > >   typedef struct {
> > > >         uint8_t layer:3;
> > > >         uint8_t crc:1;
> > > >         uint8_t channel_mode:4;
> > > >         uint8_t rfa:1;
> > > >         uint8_t mpf:1;
> > > >         uint8_t frequency:6;
> > > >         uint16_t bitrate;
> > > >   } __attribute__ ((packed)) a2dp_mpeg_t;
> > > >
> > > > But as you can see, bitrate on big endian definition is incorrect. It
> > > > is still in little endian. Proper way would be to define bitrate as
> > > > "uint8_t bitrate[2];" or as "uint8_t bitrate_lo; uint8_t bitrate_hi".
> > > >
> > >
> > > Obviously one need to convert the value when receiving, or perhaps you are
> > > saying it makes no sense to use bit fields in that case? Anyway when
> > > accessing this the user must convert to host order of he intends to use the
> > > value.
> >
> > Yes, that is the point, user needs to convert values. But they are
> > marked with types uint32_t and uint16_t, which are types normally used
> > for host endian. This just confuse people who copy+paste this file to
> > other projects.
> 
> packed struct are normally used just for casting using the received
> buffer, so they should never be used as in host order but perhaps that
> is not clearer for everyone.

Hi!

I looked into A2DP specification (a2dp_v1.3.1.pdf page 27 table 4.13)
and apparently, bitrate uses only 15 bits (not all 16 from uint16_t) and
moreover, it is in *big endian* ordering, not little endian. This was
surprise for me. But looking at it deeply, bitrate for AAC is in big
endian too and a2dp-codecs.h has already correct conversion macros from
host endian to big endian.

And it is not bitrate, but "bitrate index". In MPEG standard is then for
each Layer 1 - 3 defined conversion table from "bitrate index" to real
bitrate.

Therefore it is already broken on little endian systems (as in A2DP
buffer it is in big endian) and also on big endian systems (for Layer 1
and Layer 2 as index constants are hardcoded for Layer 3).

In few minutes I will send patch series, which fixes this problem.

> > So if correct way is to always convert these values, what about creating
> > some typedef uint16_t le_uint16_t or add comments that those values are
> > in little endian and not in host endian?
> >
> > Or usage of __attribute__((packed, scalar_storage_order("little-endian")))?
> 
> Never used the scalar_storage_order, perhaps it would be a nice
> addtion here, not sure if that does the convertion implicitly?

Yes, this is new feature of gcc6, it does implicit conversion.

In my patch series I did not used it as I do not think that bluez has
minimal required gcc version.

> > > So basically this file a2dp-codecs.h is not compatible with big endian
> > > > systems, even there are "#elif __BYTE_ORDER == __BIG_ENDIAN" branches.
> > > >
> > > > If you are not going to support big endian systems, I suggest you to
> > > > drop all those "#elif __BYTE_ORDER == __BIG_ENDIAN" parts as people
> > > > think that existence of these macros means that code is supported on big
> > > > endian systems.
> 
> It is indeed broken for big endian if no convertion is performed, the
> use o bitfield actually makes a mess in the structs so perhaps it is
> better to use just full bytes and perhaps have macros for accesing the
> bit fields, anyway we don't use them anymore in the daemon itself so
> it is more of an issue in the likes of PulseAudio, neverthless we
> should probably have proper definitions anyway in case anyone use them
> as a reference.

I will try to fix structures, so accessing them on both little and big
endian systems would result in same data. I think this is the best
approach as lot of people develops on little endian systems and if
protocol buffer endianity matches host endianity, nothing is broken and
nobody spot problem.

> > > > It is better throw an error that big endian is broken as trying to
> > > > produce non-working binaries.
> > > >
> > > > Moreover, some people started copying this a2dp-codecs.h file to other
> > > > projects which just leads to copy+paste of broken code.
> > > >
> > > > --
> > > > Pali Rohár
> > > > pali.rohar@gmail.com
> > > >
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]