Repository navigation
Improper use of signed integers #3
Description
The millis() function returns an unsigned long integer. Implicitly casting it to a signed long is a terrible idea, for several reasons:
- After roughly 24.9 days, the most significant bit of
millis()is set, meaning the cast will result in a negative number, and all theif(time < 0)tests will evaluate to true. last_run + intervalcan overflow, and overflowing a signed number in C++ yields undefined behavior, which is always a bug.- In the lucky event that the overflow yields the expected wrap-around,
_cached_next_runwill be negative and the testtime >= _cached_next_runwill immediately evaluate to true, and continue to do so as long astimeis positive.
All these issues can be avoided by using the proper type for timestamps: unsigned long.
Activity
ivanseidel commentedon Jun 11, 2015
Hellow @edgar-bonet,
You are correct. However, the idea behind
if(time < 0)is that you can either specify the time during it's call, or, set it yourself.The total 24.9 days is actually half of what would be if we set to
unsigned long, but then, the paramenter wouldn't accept a negative number (the default one is-1).Do you have any suggestions? One Idea that I had, is to set the default to
0, but that would result in a call tomillis()if the actual time was0for the first time. Not a big problem, but not perfect...edgar-bonet commentedon Jun 11, 2015
Hi!
No. If using
unsigned longthe program can run indefinitely. Unsigned integers do not really overflow: they always give the “correct” value modulo 2^(number of bits).I suggest you get rid of
_cached_next_runand decide whether it's time to run by testingThis works well across the
millis()rollover thanks to the properties of modular arithmetic. If you really need to avoid the subtraction, it is possible, but the code would be less readable.If you really need both options (specify the time or let it default to “now”), overload the methods. You cannot reserve a special time value as “invalid”, because all possible values of a 32-bit word are valid and meaningful.
edgar-bonet commentedon Jun 11, 2015
Submited a patch in the form of pull request #4.
Oh, BTW, nothing to do with the current bug but... when compiling the library, I got a warning about the variable
foundnot being used in ThreadController.cpp, line 68.ivanseidel commentedon Jun 11, 2015
fixed by #4