Possible integer overflow in OsTime

[ottonemo]

Hey,

thank you for the project, it is really useful!

While using the OsTime and OsDeltaTime interface I stumbled upon a possible integer overflow that I wanted to discuss.

Consider the following example:

OsTime time_now, time_previous;
OsDeltaTime threshold::from_ms(50);

auto active = (time_now - time_previous) >= threshold;

Internally subtraction is defined as

constexpr OsDeltaTime operator-(OsTime const &a, OsTime const &b) {
  return OsDeltaTime(a.tick() - b.tick());
}

Now the backing type of OsDeltaTime is int32 while OsTime uses uint32. If OsTime a is >2**31 the above statement would result in OsDeltaTime((int32) 2**31+1 - 0) which would clearly overflow int32 and be a negative delta time even though it would need to be positive.

Currently I'm using raw tick values to work around this ((time_now.tick() - time_previous.tick()) >= (uint32_t) threshold) but it might lead to unexpected behavior down the line so I thought it is worthwhile reporting it.

[ngraziano]

Hi, English is not my native language, I'm not sure I will be able to explain it clearly.

It is by design that there is this overflow. You must remember that OsTime will overflow regularly (every 19 hours). and sometime b is greate than a in a-b, for exemple if we have a "due time" and "current time", sometime due time is less than current time. We can't compare directly OsTime.tick() we need to compare the difference and cast it to int32

Some case:

• due_time = 0x8 , current_time = 0x01 => due time not passed, 7 tick to go
• due_time = 0x8 , current_time = 0xFFFFFFFF => due time not passed, 9 tick to go
• due_time = 0xFFFFFFFE , current_time = 0xFFFFFFFF => due time passed, 1 tick late (-1)

• due_time = 0xFFFFFFFE , current_time = 0x2 => due time passed, 4 tick late (-4)

  see test here

The only problem is when difference is greater than 9h.

Using your method ((time_now.tick() - time_previous.tick()) >= (uint32_t) threshold) will only work if time_now is always after time_previous.

[ottonemo]

Fair :) Thanks for explaining!