Tcpflow fails on some ARM an SPARC chips

[dkogan]

Hi.

I'm seeing problems with tcpflow built on ARM and SPARC:

https://buildd.debian.org/status/package.php?p=tcpflow

The build log shows that test1.sh failed. I traced the issue to unaligned memory loads. Specifically,

1. tcpdemux::process_tcp() is called with some arbitrary ip_data bytes.

2. process_tcp() does this:

   struct be13::tcphdr *tcp_header = (struct be13::tcphdr *) ip_data;

   Here we reinterpret the ip_data bytes as a tcphdr structure. This structure can have ANY alignment since it came from an array of bytes

3. When we reference this structure (for instance in tcp_header->th_seq), C assumes ALIGNED data, as it always does.

The specific behavior of unaligned memory access varies by architecture. On x86 things work, although there is a performance hit. On ARM the behavior appears to be undefined. Some chips do what you would expect, while some others assume the address is aligned, and thus end up accessing the wrong memory. On SPARC, you get a bus error and the program crashes.

Some ways to resolve this:

• we can make sure the input data is aligned
• we can mark be13::tcphdr as attribute((packed)). This will cause all accesses to this structure to happen a byte at a time, which would fix the issue, but a performance hit would result any time anything is done with this structure

[simsong]

Can you get me access to an arm system for testing?

Sent from my iPad

On Jan 13, 2014, at 12:57 AM, Dima Kogan [email protected] wrote:

Hi.

I'm seeing problems with tcpflow built on ARM and SPARC:

https://buildd.debian.org/status/package.php?p=tcpflow

The build log shows that test1.sh failed. I traced the issue to unaligned memory loads. Specifically,

tcpdemux::process_tcp() is called with some arbitrary ip_data bytes.

process_tcp() does this:

struct be13::tcphdr *tcp_header = (struct be13::tcphdr *) ip_data;

Here we reinterpret the ip_data bytes as a tcphdr structure. This structure can have ANY alignment since it came from an array of bytes

When we reference this structure (for instance in tcp_header->th_seq), C assumes ALIGNED data, as it always does.

The specific behavior of unaligned memory access varies by architecture. On x86 things work, although there is a performance hit. On ARM the behavior appears to be undefined. Some chips do what you would expect, while some others assume the address is aligned, and thus end up accessing the wrong memory. On SPARC, you get a bus error and the program crashes.

Some ways to resolve this:

we can make sure the input data is aligned

we can mark be13::tcphdr as attribute((packed)). This will cause all accesses to this structure to happen a byte at a time, which would fix the issue, but a performance hit would result any time anything is done with this structure

— Reply to this email directly or view it on GitHub.

[simsong]

Hi, Dima. Thanks for the feedback on this.

Clearly, I need to remove this:

#pragma GCC diagnostic ignored "-Wcast-align"

And it it to compile without.

That will take some effort, but I shall do it.

Can you get me access to an ARM or SPARC based system for testing?

Thanks,

Simson

On Jan 13, 2014, at 12:57 AM, Dima Kogan [email protected] wrote:

Hi.

I'm seeing problems with tcpflow built on ARM and SPARC:

https://buildd.debian.org/status/package.php?p=tcpflow

The build log shows that test1.sh failed. I traced the issue to unaligned memory loads. Specifically,

tcpdemux::process_tcp() is called with some arbitrary ip_data bytes.

process_tcp() does this:

struct be13::tcphdr *tcp_header = (struct be13::tcphdr *) ip_data;

Here we reinterpret the ip_data bytes as a tcphdr structure. This structure can have ANY alignment since it came from an array of bytes

When we reference this structure (for instance in tcp_header->th_seq), C assumes ALIGNED data, as it always does.

The specific behavior of unaligned memory access varies by architecture. On x86 things work, although there is a performance hit. On ARM the behavior appears to be undefined. Some chips do what you would expect, while some others assume the address is aligned, and thus end up accessing the wrong memory. On SPARC, you get a bus error and the program crashes.

Some ways to resolve this:

we can make sure the input data is aligned

we can mark be13::tcphdr as attribute((packed)). This will cause all accesses to this structure to happen a byte at a time, which would fix the issue, but a performance hit would result any time anything is done with this structure

— Reply to this email directly or view it on GitHub.

[dkogan]

[email protected] writes:

Hi, Dima. Thanks for the feedback on this.

Hello again, Simson.

Clearly, I need to remove this:

#pragma GCC diagnostic ignored "-Wcast-align"

And it it to compile without.

That will take some effort, but I shall do it.

To be clear, the issue isn't build warnings or errors, it's test failures. Maybe extra diagnostics would help find such issues, but they wouldn't fix them.

Also, I patched my package by adding attribute((packed)), and all the tests now pass:

https://buildd.debian.org/status/package.php?p=tcpflow

Additionally, attribute((packed)) is already used liberally in bulk_extractor_i.h, so adding more of such tags is in line with what the rest of that code does. There're a few structures left that aren't packed. I haven't looked at the users of these structures to determine if this is ok. Those are

struct ip4_dgram struct ip6_addr struct ip6_dgram

In any case, the tests pass even without packing those.

Can you get me access to an ARM or SPARC based system for testing?

Sadly I cannot. I'm using Debian's machines for this, and I'm not allowed to give out access. To make this even more annoying, not all ARM chips appear susceptible. I tried with my phone (ARMv7) and it appeared to work OK. The Debian porterbox is affected. It's an ARMv5.

I haven't looked too deeply at the sparc situation, but it had the same issue. I have no non-debian sparc hardware at all, and haven't tried the emulator. Here's a simple program to see if a machine allows unaligned access:

#include <stdint.h> #include <stdio.h> struct S { uint16_t a,b; uint32_t c,d; }; int main(void) { unsigned char bytes[] attribute((aligned(4))) = {0,0, 1,2,3,4, 5,6,7,8, 9,9,9,9}; struct S* s = (struct S*)(bytes+2); printf("0x%x\n", s->c); return 0; }

If unaligned is allowed, this says 5678, otherwise, it can say anything else. On the affected ARM it said 3456.

[simsong]

Dima,

I understand that the issue is test failures. However, the failures were predicted by the warnings, which I disabled. I will undisable the warnings and fix them. When the warnings are gone, the problem should be resolved.

Thanks!

[simsong]

I’ve looked at the code. Looks like the right thing to do is to have all of the array elements actually be accessor functions. Sounds like your fix is the right way to go for now.

On Jan 13, 2014, at 12:57 AM, Dima Kogan [email protected] wrote:

Hi.

I'm seeing problems with tcpflow built on ARM and SPARC:

https://buildd.debian.org/status/package.php?p=tcpflow

The build log shows that test1.sh failed. I traced the issue to unaligned memory loads. Specifically,

tcpdemux::process_tcp() is called with some arbitrary ip_data bytes.

process_tcp() does this:

struct be13::tcphdr *tcp_header = (struct be13::tcphdr *) ip_data;

Here we reinterpret the ip_data bytes as a tcphdr structure. This structure can have ANY alignment since it came from an array of bytes

When we reference this structure (for instance in tcp_header->th_seq), C assumes ALIGNED data, as it always does.

The specific behavior of unaligned memory access varies by architecture. On x86 things work, although there is a performance hit. On ARM the behavior appears to be undefined. Some chips do what you would expect, while some others assume the address is aligned, and thus end up accessing the wrong memory. On SPARC, you get a bus error and the program crashes.

Some ways to resolve this:

we can make sure the input data is aligned

we can mark be13::tcphdr as attribute((packed)). This will cause all accesses to this structure to happen a byte at a time, which would fix the issue, but a performance hit would result any time anything is done with this structure

— Reply to this email directly or view it on GitHub.