Hi Diego, On 06/01/2013 07:48 PM, Diego Biurrun wrote:
On 2013-06-01 17:43, Miika Komu wrote:Hi, On 05/31/2013 06:58 PM, Diego Biurrun wrote:On 2013-05-31 09:43, Miika Komu wrote:On 05/26/2013 07:51 PM, Diego Biurrun wrote:On 2013-05-26 18:16, Diego Biurrun wrote:On Sat, May 25, 2013 at 01:37:18PM -0000, noreply@xxxxxxxxxxxxx wrote:--- libcore/gpl/checksum.c 2012-05-12 06:54:33 +0000 +++ libcore/gpl/checksum.c 2013-05-25 13:36:48 +0000 @@ -228,7 +228,7 @@ uint16_t checksum = 0; unsigned long sum = 0; int count = 0, length = 0; - unsigned short *p = NULL; /* 16-bit */ + static unsigned short *p = NULL; /* 16-bit */ struct pseudo_header pseudoh = { { 0 } }; struct pseudo_header6 pseudoh6 = { { 0 } }; uint32_t src_network, dst_network;I think this makes the situation worse, not better. This code is broken and now you rely on some gcc optimization (or lack thereof) to hide the fact. Have you verified this even works with more than one gcc version (not to mention other compilers)? Since the bug appears with 32 and 64 bit platforms, one needs to find out why different checksums are generated in the first place. Some parts hip_checksum_packet() must have hidden assumptions about pointer or integer type sizes. Also note the rampant casting that is used to paper over design issues. My money is on the pointer dereferences sum += *p++; count -= 2; should depend on the size of the pointer, which can be 32 or 64 bits, so count cannot be decremented by the fixed size of '2', which is only correct for 32 bits.Hmmm, no, that analysis is not correct. The decrement should work. I suggest starting to debug this by getting rid of all the short and long integer types used in that file and replace them by POSIX types like uint16_t that have guaranteed sizes.I tried this, but it didn't help, so the problem originates from gcc optimizations.No, the problem is in the code, not in gcc.the code works if I disable gcc optimizations. If you find it more suitable, I can replace the static variable by adding __attribute__((optimize("O0"))) around the function?That would be better as it makes your intention much more explicit. Normally static variables are thought of as values that don't live on the stack and survive function calls. It's absolutely not obvious that here the "static" keyword is (mis)used to trick gcc into turning off optimizations, which then magically fixes a checksum mismatch bug.
done.
Why not commit your changes or send as a patch?What do you mean?Maybe I misunderstood, but it sounded as if you had (partially) converted that file to fixed-size integer types. If you did, then I think that would be worth committing, independent of whether or not it fixes the bug.
Now committed.
Also, might there be a problem with struct padding?Possibly, any concrete suggestions what to try and test?You could try to see what happens if you mark the structs in checksum.c as attribute(packed). I would guess that currently all the members are aligned to int boundaries.
Didn't help.
P.S.: Have you tried asking Boeing to relicense their code for us?I think we don't need an Aalto/RWTH-specific copyright file. We could just add a new file (e.g. hip_checksum.c), copy the BSD/MIT boilerplate from Boeing along with the function to the new file.Yes. My point is: What code from Boeing remains and have you asked them to relicense it to MIT? If they did relicense, then IIUC that would eliminate most of the GPL code in HIPL, right?
Boeing has already relicensed their code to MIT. I have reflected this already in user_ipsec_esp.c. I think we can also close this bug by changing the copyright boilerplate and moving the checksum.c away from the gpl directory:
https://bugs.launchpad.net/hipl/+bug/682323 These will still need some work: https://bugs.launchpad.net/hipl/+bug/697214 https://bugs.launchpad.net/hipl/+bug/697216 https://bugs.launchpad.net/hipl/+bug/697223
