fix(HardwareTimer): guard HERTZ_FORMAT get* against register==0#3020
Open
94xhn wants to merge 1 commit into
Open
fix(HardwareTimer): guard HERTZ_FORMAT get* against register==0#302094xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
getCount(HERTZ_FORMAT) and getCaptureCompare(..., HERTZ_COMPARE_FORMAT) both compute getTimerClkFreq() / (register * Prescalerfactor), dividing directly by the live CNT (counter) or CCR (capture/compare) hardware register with no zero-guard. Both registers legitimately read 0 with no caller error involved: - CNT is 0 right at/after every timer reset or overflow, simply from the timer running (getOverflow()/setOverflow() already special-case this exact situation via the ARR+1 "period" offset; getCount() and getCaptureCompare() do not). - CCR is 0 on reset before the first setCaptureCompare() call, and in Input Capture mode holds whatever was last captured, which is 0 before any edge has been captured or if an edge lands exactly at CNT == 0. Hitting either case triggers an integer divide by zero: undefined behavior in C/C++, verified to crash (SIGFPE-equivalent) on a desktop toolchain. On Cortex-M without DIV_0_TRP configured (the default), UDIV silently returns 0, so on-target this instead silently reports "0 Hz" with no indication anything went wrong. Add an explicit zero-check to both HERTZ_FORMAT branches, returning 0 (no meaningful frequency available) instead of dividing, matching the existing "do not underflow" zero-guard style already used by setOverflow() in this same file. No public API/signature changes. Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request template
Summary
HardwareTimer::getCount(HERTZ_FORMAT)andHardwareTimer::getCaptureCompare(channel, HERTZ_COMPARE_FORMAT)both convert a hardware register value to a frequency by dividinggetTimerClkFreq()by that register value. Neither has a zero-guard, and both registers legitimately read0during normal operation (not because of a caller mistake), so both are reachable divide-by-zero bugs.This PR fixes/implements the following bugs/features
getCount(HERTZ_FORMAT)divides by the liveCNT(counter) register, which is0right at/after every timer reset or overflow, purely as a side effect of the timer running.getCaptureCompare(channel, HERTZ_COMPARE_FORMAT)divides by the liveCCR(capture/compare) register, which is0on reset before the firstsetCaptureCompare()call, and in Input Capture mode whenever no edge has been captured yet or the last captured edge landed exactly atCNT == 0.Motivation
In
libraries/SrcWrapper/src/HardwareTimer.cpp:Neither
CNT_RegisterValuenorCCR_RegisterValueis validated to be non-zero before the division, and0is not an edge case a caller passes in — it's a value the hardware itself produces during ordinary operation:CNT(the running counter) counts0, 1, 2, ..., ARR, 0, 1, 2, ...— it reads0once every single timer period. NotegetOverflow()/setOverflow()already handle the analogous "register can be one less than the real period" situation via the documentedARR + 1offset;getCount()has no equivalent protection.CCR(capture/compare register) is0immediately afterbegin()/setMode()before anysetCaptureCompare()call, and in Input Capture mode it holds the last captured counter value, which is0before the first edge is captured or whenever an edge happens to land exactly whenCNT == 0.Integer division by zero is undefined behavior in C/C++. On this project's usual desktop/CI toolchains it reliably crashes; on Cortex-M targets,
UDIV/SDIVby0is architecturally defined to return0whenDIV_0_TRPis not set inSCB->CCR(the default, and not touched anywhere in this codebase), so on-target the bug doesn't crash — it silently returns0 Hz/no fault, which is arguably worse since it looks like a valid (if surprising) reading instead of an obvious error.This is the same general bug shape as the
getCaptureCompare()PERCENT_COMPARE_FORMAT/RESOLUTION_xB_COMPARE_FORMATdivide-by-zero I reported separately in #3019 (dividing by a hardware register that can be0), found while re-reading the same conversion functions for otherHERTZ_FORMAT/HERTZ_COMPARE_FORMATsiblings — this is an independent code path (differentcasebranches, different registers) so I've kept it as its own PR.Fix
Add an explicit
== 0guard to bothHERTZ_FORMATbranches and return0(no meaningful frequency available) instead of dividing, following the same "do not underflow, default to a safe value" stylesetOverflow()already uses a few lines above in this file. No public API/signature changes.Validation
Ensure CI build is passed.
This is pure integer arithmetic on values that are otherwise plain
uint32_ts once read through the existingLL_TIM_*abstraction, so I extracted both formulas verbatim into a standalone host-side C program and ran it on a desktop toolchain (independent of any STM32 board):Running the buggy formula directly (
getCount_HERTZ_BUGGY(0, 1)) crashes the host process immediately (process exits before the nextprintf, noSIGFPEhandler installed) — confirming the divide-by-zero is real, not just theoretical, for both formulas. The fixed formula returns a defined0in the same case and is unchanged for every non-zero register value tested.I do not own STM32 hardware set up for this project's test rig, so I have not run this on-target; the change only adds a zero-guard around a pure integer division that is otherwise structurally identical to the surrounding, already-shipped code, so on-target risk is limited to the two
== 0branches added.Code formatting
Closing issues
N/A – found while reading the timer tick/frequency conversion code, not tied to an existing issue.
Disclosure: I used an AI coding assistant (Claude) to help read through the codebase, extract the reproduction, and draft this PR. All findings were verified by me by compiling and running the standalone reproduction shown above.