32-bit integer wrapping in PolicyEngine::getTimeStamp() not properly handled
nyanpasu64 opened this issue · 0 comments
A few days ago, in the Pinecil chat, I brought up the possibility that after 49 days, FreeRTOS's 64-bit millisecond tick counter would overflow after 49.7 days of being plugged in, if casted to 32 bits by IronOS. Ralim/IronOS@ba2724b fixed this issue for the most part. The only remaining use of 32-bit millisecond counts is in https://github.com/Ralim/IronOS/blob/dev/source/Core/Drivers/USBPD.cpp#L22 uint32_t get_ms_timestamp()
. This is passed into usb-pd's PolicyEngine::getTimeStamp
.
At this point, I audited uses of PolicyEngine::getTimeStamp
for bugs. VS Code clangd found uses in https://github.com/Ralim/usb-pd/blob/main/src/policy_engine.cpp and https://github.com/Ralim/usb-pd/blob/main/src/policy_engine_states.cpp. Most uses subtract getTimeStamp() - earlier time
, which is safe since unsigned integers are modulo in C++.
However, I found that src/policy_engine.cpp#PolicyEngine::waitForEvent()
assigns waitingEventsTimeout = getTimeStamp() + timeout;
. If getTimeStamp()
is close to 2^32, it's possible (waitingEventsTimeout = getTimeStamp() + timeout) < getTimeStamp()
. Subsequently, when src/policy_engine_states.cpp#PolicyEngine::pe_sink_wait_event()
runs if (getTimeStamp() > waitingEventsTimeout)
, this check can immediately timeout since waitingEventsTimeout < getTimeStamp()
.
This appears to be a logic error. Is it a problem in practice (for IronOS or this library in general)?