paho.mqtt.c on powerpc (big endian) fails to send connect command message to broker

[JuergenKosel]

mqtt-powerpc.pcap.pcap.zip

Hello,

I have build paho.mqtt.c async library for a powerpc-linux device (big endian). But the client fails to establish the connection to the broker. As the attached wireshark capture shows, the client sends a broken connect command message to the broker. So the broker closes the tcp connection.

Greetings Juergen

[JuergenKosel]

Hello,

I have found the key to the problem in https://github.com/eclipse/paho.mqtt.c/blob/master/src/MQTTPacket.h#L54

The macro REVERSED needs to be defined for big endian machines and must not be defined for little endian machines. So my solution would be:

1. include endian.h
2. rename REVERSED to BIG_ENDIAN_HOST
3. define BIG_ENDIAN_HOST depending on __BYTE_ORDER == __BIG_ENDIAN

Greetings Juergen

[JuergenKosel]

Hello,

a fix is provided in commit c1ccb544718ed5bea6440968eeebd30754c99192 , but this depends on the auto-tools pull request #148 . Because endian.h does not exist for Windows and maybe others. autoconf could find out, if endian.h could be included. (I do not know how to do this with cmake.)

Greetings Juergen

[JuergenKosel]

Hello,

I have just seen issue #77 which is the same issue as this one.

[icraggs]

Juergen, I think it's possible that noting the endianness might not be reliable enough, because the order of bitfields is not guaranteed by the C standard, the last time I looked.

I did want to use bitfields originally because it seemed an ideal use case rather than using bit operations, but maybe bit operations would be more reliable and we wouldn't need this at all.

Alternatively I had envisaged the possibility of some piece of code run during the build which would work out whether REVERSED was needed or not.

[JuergenKosel]

Hello,

the Linux header files include/linux/byteorder/big_endian.h and include/linux/byteorder/little_endian.h define the macros BIG_ENDIAN_BITFIELD and LITTLE_ENDIAN_BITFIELD. So I assume, that at least for the gcc the order of the bit-fields depends on the byte order. And even it is not written in the C-standard, it may be really difficult for a complier to not depend the order of the bit fields on the byte order. E.g. for bit fields on 16 bits with a bit field partly in the high and the low byte.

[icraggs]

I've added a section to MQTTPacket.h

if defined(linux)

include

if __BYTE_ORDER == __BIG_ENDIAN

   #define REVERSED 1

endif

endif

which should do the job for Linux.

[icraggs]

Will be in 1.2.

[daniel-santos]

I've added a section to MQTTPacket.h

if defined(linux)

include

if __BYTE_ORDER == __BIG_ENDIAN

define REVERSED 1

endif

endif

which should do the job for Linux.

Hello. Sorry for my late contribution here, but the the notion that endianess dictates bitfield order is mistaken and any such observation coincidental.

As you may be aware, no version of the C standard defines the layout of bitfields, so it remains entirely up to the compiler. This is unfortunate, because it renders them unsafe for any data exchanged with other programs or libraries, since we cannot be guaranteed which compiler it was built with, and thus, how the compiler laid out the bitfields.

I attempted to resolve this last year in a way that would generate conversion code when the assumed bit layout was wrong, but "compile-out" (into a no-op) when the assumption was correct, but my efforts were thwarted by a gcc optimization flaw (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83784).

So absent some data translation prior to sharing the bitfield externally, I strongly recommend not using them for any data that is ever shared outside of the program and instead replace such code with old, clunky and verbose macros and/or inline functions.

Example (untested):

typedef char Header;

#define HEADER_RETAIN_SHIFT 0
#define HEADER_RETAIN_BITS  1
#define HEADER_QOS_SHIFT    1
#define HEADER_QOS_BITS     2
#define HEADER_DUP_SHIFT    3
#define HEADER_DUP_BITS     1
#define HEADER_TYPE_SHIFT   4
#define HEADER_TYPE_BITS    4

#define header_get1(v) v
#define header_get0(v) header_get1(v)
#define header_get(field, h) header_get0( \
    ((h) >> HEADER_##field##_SHIFT) & ((1 << HEADER_##field##_BITS) - 1))

#define header_set1(v) v
#define header_set0(v) header_set1(v)
#define header_set(field, h, v) header_set0( \
    (((h) & ~((1 << HEADER_##field##_BITS) - 1) << HEADER_##field##_SHIFT) \
    | ((v) >> HEADER_##field##_SHIFT)))

It's ugly, but it's better than unsafe.

EDIT: May I should get it together and write a proposal for C2x.