Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

32-bit integer wrapping in PolicyEngine::getTimeStamp() not properly handled #14

Open
nyanpasu64 opened this issue Jul 27, 2022 · 0 comments

Comments

@nyanpasu64
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant