Ralim/usb-pd

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)?