Wednesday, August 4, 2010

Patch: airbase-ng timestamp bug

I'm diagnosing another major bug with airbase-ng. Every so often, about 1% of the time, things stop working for no apparent reason. I pulled out Wireshark to see what what was going on. I noticed a minor flood of packets that looked like the following:

As you can see, the "Timestamp" field is obviously bogus, having gone negative. Just by looking at the field, the cause of this is intuitively obvious. It must be some sort of 32-bit to 64-bit integer promotion bug (the field is 64-bits wide, the code runs on a 32-bit version of Linux).

Hunting this down was quick, such search for "gettimeofday", "timestamp", or "tv1". The offending code looks like this:
struct timeval tv1;
uint64_t timestamp;
...
gettimeofday( &tv1, NULL );
timestamp=tv1.tv_sec*1000000 + tv1.tv_usec;

Yep, that's obviously a 32-to-64 promotion (tv_sec is defined as an int). Multiplying by a million causes the value to fill out the full 32-bits, setting the high-order bit, meaning the number is now negative. When the 32-bit result gets assigned to a 64-bit integer, it is "sign-extended", filling out all the high-order bits.

There are many possible fixes, but the simplest is just to change the constant from a 32-bit value to a 64-bit value. In C, appending the letter "L" to a number makes it 64-bits, so whereas the number "1" is 32-bits, "1L" is 64-bits. Appending the letter "U" makes it unsigned, preventing sign extension. Thus, "1UL" is a 64-bit unsigned number with the value of "1".

This can easily be tested with the following program:
#include <stdio.h>
#include <stdint.h>
#include <sys/time.h>
int main()
{
    struct timeval tv1;
    uint64_t timestamp;
    for (;;) 
    {
        gettimeofday(&tv1, NULL);
        timestamp = tv1.tv_sec * 1000000 + tv1.tv_usec;
        printf("+%llx\n", timestamp);
        timestamp = tv1.tv_sec * 1000000UL + tv1.tv_usec;
        printf(" %llx\n", timestamp);
        sleep(1);
    }
}

If you let it run during the time that the expression goes negative, you'll get the following result (hex numbers starting with 89ABCDEF are negative):
+ffffffffe78b00d5
 e78b00d5
+ffffffffe79a6a3d
 e79a6a3d
+ffffffffe7a9d355
 e7a9d355
+ffffffffe7b932e0
 e7b932e0
+ffffffffe7c87784
 e7c87784
+ffffffffe7d7c11a
 e7d7c11a
+ffffffffe7e70493
 e7e70493
+ffffffffe7f65dc1
 e7f65dc1

The expression exists in several places in airbase-ng.c. The solution is to go through the source and added the 'L' to the end of each constant:
-    timestamp=tv1.tv_sec*1000000 + tv1.tv_usec;
+    timestamp=tv1.tv_sec*1000000UL + tv1.tv_usec;

After I do that, this problem goes away.

Note that this isn't the best fix -- it's just the one that fixes the problem with the least amount of fuss.

No comments:

Post a Comment