Endianness of a single byte: big or little?
Bug
Rolf Eike Beer noticed two test failures while testing radvd package (IPv6 route advertiser daemon and more) on sparc. Both tests failed likely due to endianness issue:
test/send.c:317:F:build:test_add_ra_option_lowpanco:0:
Assertion '0 == memcmp(expected, sb.buffer, sizeof(expected))'
failed: 0 == 0, memcmp(expected, sb.buffer, sizeof(expected)) == 1
test/send.c:342:F:build:test_add_ra_option_abro:0:
Assertion '0 == memcmp(expected, sb.buffer, sizeof(expected))'
failed: 0 == 0, memcmp(expected, sb.buffer, sizeof(expected)) == 1
I’ve confirmed the same failure on powerpc.
Eike noted that it’s unusual because sparc is a big-endian architecture and network byteorder is also big-endian (thus no need to flip bytes). Something very specific must have lurked in radvd code to break endianness in this case. Curiously all the radvd tests were working fine on amd64.
Two functions failed to produce expected results:
(test_add_ra_option_lowpanco)
START_TEST{
(0, iface);
ck_assert_ptr_ne
struct safe_buffer sb = SAFE_BUFFER_INIT;
(&sb, iface->AdvLowpanCoList);
add_ra_option_lowpanco
unsigned char expected[] = {
0x22, 0x03, 0x32, 0x48, 0x00, 0x00, 0xe8, 0x03, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
};
(sb.used, sizeof(expected));
ck_assert_int_eq(0, memcmp(expected, sb.buffer, sizeof(expected)));
ck_assert_int_eq
(&sb);
safe_buffer_free}
END_TEST
(test_add_ra_option_abro)
START_TEST{
(0, iface);
ck_assert_ptr_ne
struct safe_buffer sb = SAFE_BUFFER_INIT;
(&sb, iface->AdvAbroList);
add_ra_option_abro
unsigned char expected[] = {
0x23, 0x03, 0x0a, 0x00, 0x02, 0x00, 0x02, 0x00, 0xfe, 0x80, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0xa2, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
};
(sb.used, sizeof(expected));
ck_assert_int_eq(0, memcmp(expected, sb.buffer, sizeof(expected)));
ck_assert_int_eq
(&sb);
safe_buffer_free}
END_TEST
Both tests are straightforward: they verify 6LoWPAN and ABRO extension handling (both are related to route announcement for Low Power devices). Does not look complicated.
I looked at add_ra_option_lowpanco() implementation and noticed at least one bug of missing endianness conversion:
// radvd.h
struct nd_opt_abro {
uint8_t nd_opt_abro_type;
uint8_t nd_opt_abro_len;
uint16_t nd_opt_abro_ver_low;
uint16_t nd_opt_abro_ver_high;
uint16_t nd_opt_abro_valid_lifetime;
struct in6_addr nd_opt_abro_6lbr_address;
};
// ...
// send.c
static void add_ra_option_mipv6_home_agent_info(struct safe_buffer *sb, struct mipv6 const *mipv6)
{
struct HomeAgentInfo ha_info;
(&ha_info, 0, sizeof(ha_info));
memset
.type = ND_OPT_HOME_AGENT_INFO;
ha_info.length = 1;
ha_info.flags_reserved = (mipv6->AdvMobRtrSupportFlag) ? ND_OPT_HAI_FLAG_SUPPORT_MR : 0;
ha_info.preference = htons(mipv6->HomeAgentPreference);
ha_info.lifetime = htons(mipv6->HomeAgentLifetime);
ha_info
(sb, &ha_info, sizeof(ha_info));
safe_buffer_append}
static void add_ra_option_abro(struct safe_buffer *sb, struct AdvAbro const *abroo)
{
struct nd_opt_abro abro;
(&abro, 0, sizeof(abro));
memset
.nd_opt_abro_type = ND_OPT_ABRO;
abro.nd_opt_abro_len = 3;
abro.nd_opt_abro_ver_low = abroo->Version[1];
abro.nd_opt_abro_ver_high = abroo->Version[0];
abro.nd_opt_abro_valid_lifetime = abroo->ValidLifeTime;
abro.nd_opt_abro_6lbr_address = abroo->LBRaddress;
abro
(sb, &abro, sizeof(abro));
safe_buffer_append}
Note how add_ra_option_mipv6_home_agent_info() carefully flips bytes with htons() for all uint16_t fields but add_ra_option_abro() does not.
It means the ABRO does not really work on little-endian (aka most) systems in radvd and test checks for the wrong thing. I added missing htons() calls and fixed expected[] output in tests by manually flipping two bytes in a few locations.
Plot twist
The effect was slightly unexpected: I fixed only ABRO test, but not 6LoWPAN. It’s where things became interesting. Let’s look at add_ra_option_lowpanco() implementation:
// radvd.h
struct nd_opt_6co {
uint8_t nd_opt_6co_type;
uint8_t nd_opt_6co_len;
uint8_t nd_opt_6co_context_len;
uint8_t nd_opt_6co_res : 3;
uint8_t nd_opt_6co_c : 1;
uint8_t nd_opt_6co_cid : 4;
uint16_t nd_opt_6co_reserved;
uint16_t nd_opt_6co_valid_lifetime;
struct in6_addr nd_opt_6co_con_prefix;
};
// ...
// send.c
static void add_ra_option_lowpanco(struct safe_buffer *sb, struct AdvLowpanCo const *lowpanco)
{
struct nd_opt_6co co;
(&co, 0, sizeof(co));
memset
.nd_opt_6co_type = ND_OPT_6CO;
co.nd_opt_6co_len = 3;
co.nd_opt_6co_context_len = lowpanco->ContextLength;
co.nd_opt_6co_c = lowpanco->ContextCompressionFlag;
co.nd_opt_6co_cid = lowpanco->AdvContextID;
co.nd_opt_6co_valid_lifetime = lowpanco->AdvLifeTime;
co.nd_opt_6co_con_prefix = lowpanco->AdvContextPrefix;
co
(sb, &co, sizeof(co));
safe_buffer_append}
The test still failed to match one single byte: the one that spans 3 bitfields: nd_opt_6co_res, nd_opt_6co_c, nd_opt_6co_cid. But why? Does endianness really matter within byte? Apparently gcc happens to group those 3 fields in different orders on x86_64 and powerpc!
Let’s looks at a smaller example:
#include <stdio.h>
#include <stdint.h>
struct s {
uint8_t a : 3;
uint8_t b : 1;
uint8_t c : 4;
};
int main() {
struct s v = { 0x00, 0x1, 0xF, };
("v = %#02x\n", *(uint8_t*)&v);
printfreturn 0;
}
Output difference:
$ x86_64-pc-linux-gnu-gcc a.c -o a && ./a
v = 0xf8
# (0xF << 5) | (0x1 << 4) | 0x00
$ powerpc-unknown-linux-gnu-gcc a.c -o a && ./a
v = 0x1f
# (0x0 << 5) | (0x1 << 4) | 0xF
C standard does not specify layout of bitfields and it’s a great illustration of how things break :)
An interesting observation: the bitfield order on powerpc happens to be the desired order (as 6LoWPAN RFC defines it).
It means radvd code indeed happened to generate correct bitstream on big-endian platforms (as Eike predicted) but did not work on little-endian systems. Unfortunately golden expected[] output was generated on little-endian system.
Thus the 3 fixes:
- use uint8_t instead of bitfield (and tweak the golden byte): https://github.com/reubenhwk/radvd/pull/78/commits/25f627b3ae878cab1c5362ba70947e1bf8a250c4
- regenerate golden output on big-endian system: https://github.com/reubenhwk/radvd/pull/78/commits/6b6f81e2979a3cd266e2fce84044485300460767
- fix missing uint16_t handling: https://github.com/reubenhwk/radvd/pull/78/commits/dac7758152188aed9bef140ca2132271e77dfa74
That’s it :)