ximgproc: fix uninitialised read of ellipse periphery point in ValidateCircles - #4208
Open
purehol wants to merge 1 commit into
Open
ximgproc: fix uninitialised read of ellipse periphery point in ValidateCircles#4208purehol wants to merge 1 commit into
purehol wants to merge 1 commit into
Conversation
…teCircles ValidateCircles() sets noPoints = computeEllipsePerimeter(...) (odd about half the time) and calls ComputeEllipsePoints(px, py, noPoints), which writes exactly 2*(noPoints/2) points. For an odd noPoints the last slot px/py[noPoints-1] is never written, yet the validation loop (j < noPoints) reads it: an uninitialised read for the first candidate, or a stale value left by a previous candidate (the buffers are reused), which then feeds the NFA alignment statistics. Drop the odd point before sampling, matching the reference implementation ED_Lib. Verified with a deterministic sentinel (px/py[noPoints-1] stayed unwritten for odd noPoints on a real image) and with valgrind --track-origins (uninitialised value at the ValidateCircles branches, origin at the px/py allocation); both are clean after the change. Adds a test that exercises the odd-perimeter validation path under memory-checking CI.
purehol
marked this pull request as ready for review
September 2, 2026 16:55
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.
Root cause
ValidateCircles()setsnoPoints = computeEllipsePerimeter(...)(odd about halfthe time) and calls
ComputeEllipsePoints(px, py, noPoints), which writes exactly2 * (noPoints/2)points. For an oddnoPointsthe last slotpx/py[noPoints-1]is never written, yet the validation loop
for (j = 0; j < noPoints; j++)readsit — an uninitialised read for the first candidate, or a stale value from a
previous candidate (the buffers are reused). That phantom point is then fed into
checkValidationByNFA(noPeripheryPixels, aligned).Fix
Drop the odd point before sampling (
if (noPoints % 2) noPoints--;), matching thereference implementation ED_Lib. The OpenCV port had adopted the
points_buffer_sizebound from the same upstream fix but missed this odd-countguard.
Behavior change
This is not a neutral change of output — it removes a phantom sample from the NFA
statistics for odd-perimeter ellipses. The phantom point holds an indeterminate
value (uninitialised for the first candidate, stale for later ones); it is counted
as a periphery sample and, depending on that value, is either skipped by the bounds
check (when it falls outside the image) or fully processed (when it falls inside,
where it can also change
aligned), so it perturbs the validation of everyodd-perimeter ellipse in an unpredictable direction. The effect is usually small:
over 93 OpenCV sample images plus a synthetic case, the fix changed the
detected-ellipse count on 6 of them (in both directions) and left the other 87
unchanged. Where an ellipse does change, the post-fix computation uses only the real
periphery points (the never-written slot is no longer read); we did not assess which
output is closer to ground truth. Reading the unwritten slot is also undefined
behaviour, flagged by memory sanitizers.
Verification
On stock OpenCV,
valgrind --track-originsondetectEllipses()over an imagewith several ellipses reports "uninitialised value" in
ValidateCircles(
ERROR SUMMARY: 2 errors) before the change and0 errorsafter it. Adeterministic, layout-independent confirmation (temporary source instrumentation,
not part of this change) writes a sentinel to
px/py[noPoints-1]before the calland observes it left unwritten for odd
noPoints(e.g. 375, 229). AddedTEST_F(ximgproc_ED, detectEllipsesOddPerimeterNoUninitRead)to exercise theodd-perimeter path under memory-checking CI. (valgrind flags only the
first-candidate uninitialised reads; the stale-reuse case is covered by the
write-count-vs-read-count argument above.)
Fixes #4207