Integer overflow during time calculations

[marvin2k]

After compiling with -fsanitize=undefined a brief test of the hokuyo_test binary revealed at least two locations with integer overlows at runtime (1 2). In both locations due to using a plain int for "device timestamps". As I don't know anything about the internals of the driver or Hokuyo Protocols, I'm just opening the issue instead of writing any code. Just casting the intermediary summation to int64 might be enough to fix the problem at hand?

range.time = device_time_offset+base::Time::fromMicroseconds((int64_t)device_timestamp*1000-3100);

What I don't get is how the timestamp times one thousand can get larger than two billion -- these would be two thousand seconds? Sadly I don't remember the exact values reported...

If I get access to hardware I would like to test more sanitizers.

[shoppel]

The links seem not to work, can you post both the lines the overflow should occur?

[marvin2k]

Sure, why not.

bool URG::measureCommunicationLatency(int timeout)
{
    // now try for synchronisation
    if (!simpleCommand(URG_TM0, timeout)) {
    simpleCommand(URG_TM2, 0);
    return false;
    }
    int ts;
    base::Time t1 = base::Time::now();
    if (!timeCommand(ts, timeout)) {
    simpleCommand(URG_TM2, 0);
    return false;
    }
    base::Time t2 = base::Time::now();
    if (!simpleCommand(URG_TM2, timeout))
    return false;

    // this "ts*1000" is reported
    device_time_offset = t1/2+t2/2-base::Time::fromMicroseconds(ts*1000);
    return true;
}

    // subtract 3.1 ms for the difference between "back of the scanner"
    // and measurement 0
    // and this "device_timestamp*1000" is reported.
    range.time = device_time_offset+base::Time::fromMicroseconds(device_timestamp*1000-3100);

    //period of the device
    base::Time period = base::Time::fromSeconds(1.0 / (range.speed / (M_PI * 2.0)));

    //compute the sample counter
    int sample_count_diff = 0;

Maybe disable uMatrix to make the links work? Here they do.

[jmachowinski]

How about this fix :

range.time = device_time_offset+base::Time::fromMilliseconds(device_timestamp) - base::Time::fromMicroseconds(3100);

Seems cleaner than casting around

[marvin2k]

Hm, created #7 which passes now. As said, did not check for actual functionality. Might do in the course of the day.