EdgeDrawing::ValidateCircles() reads an uninitialised ellipse point for odd perimeters

System information (version)

• OpenCV => 4.x head (reproduced on 4.14.0)
• Operating System / Platform => Ubuntu (Linux 5.15) 64 Bit
• Compiler => gcc 9.4, valgrind 3.24

Detailed description

In EdgeDrawingImpl::ValidateCircles() (modules/ximgproc/src/edge_drawing.cpp:3254 on 4.x), ComputeEllipsePoints(circle->eq.coeff, px, py, noPoints) is called with noPoints = (int)computeEllipsePerimeter(...), which is odd about half the time. ComputeEllipsePoints writes exactly 2 * (noPoints/2) entries (px[j-1] and px[j-1+npts] for j = 1..npts; edge_drawing.cpp:4975-4989), so for an odd noPoints the last slot px/py[noPoints-1] is never written. The validation loop for (j = 0; j < noPoints; j++) (:3269) nevertheless reads it:

• for the first candidate the px/py buffers are freshly new-allocated, so this is a read of uninitialised memory (an indeterminate value — undefined behaviour);
• for later candidates the buffers are reused, so it reads a stale value left by a previous candidate.

That phantom point is then fed into the NFA validation: it is counted as a periphery sample (noPeripheryPixels++, :3276), and depending on its indeterminate coordinates is either skipped by the bounds check (:3278-3281, when it falls outside the image) or fully processed (when it happens to fall inside, where it can also change aligned). Either way the decision checkValidationByNFA(noPeripheryPixels, aligned) (:3440) is computed from corrupted statistics for every odd-perimeter ellipse, and the direction of the perturbation is not predictable.

Impact: reading the unwritten slot is undefined behaviour and is flagged by memory sanitizers. The behavioural effect is usually small: in our testing over 93 OpenCV sample images plus a synthetic case, applying the fix changed the number of detected ellipses on 6 of them (in both directions) and left the other 87 unchanged. Where it 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. (Results were deterministic across repeated runs on this build.)

The reference implementation guards the call with if (noPoints % 2) noPoints--; (CihanTopal/ED_Lib). The OpenCV port adopted the points_buffer_size bound from that same fix but missed the odd-count guard.

Steps to reproduce (stock OpenCV, no source changes)

Run detectEllipses() on an image with several ellipses (some will have an odd computed perimeter) under valgrind:

#include <opencv2/imgproc.hpp>
#include <opencv2/ximgproc.hpp>
#include <vector>
using namespace cv;

int main() {
    Mat img(400, 400, CV_8UC1, Scalar(255));
    ellipse(img, Point(120,120), Size(70,45),  20, 0, 360, Scalar(0), 2);
    ellipse(img, Point(280,150), Size(55,55),   0, 0, 360, Scalar(0), 2);
    ellipse(img, Point(200,300), Size(90,40),  60, 0, 360, Scalar(0), 2);
    ellipse(img, Point(320,320), Size(33,50), 130, 0, 360, Scalar(0), 2);
    ellipse(img, Point( 90,320), Size(48,27),  95, 0, 360, Scalar(0), 2);
    Ptr<ximgproc::EdgeDrawing> ed = ximgproc::createEdgeDrawing();
    ed->detectEdges(img);
    std::vector<Vec6d> ellipses;
    ed->detectEllipses(ellipses);
    return 0;
}

$ valgrind --track-origins=yes ./a.out
==...== Conditional jump or move depends on uninitialised value(s)
==...==    at 0x...: cv::ximgproc::EdgeDrawingImpl::ValidateCircles(bool)
==...==  Uninitialised value was created by a heap allocation
==...==    at 0x...: cv::ximgproc::EdgeDrawingImpl::ValidateCircles(bool)   // the px/py new double[]
==...== ERROR SUMMARY: 2 errors from 2 contexts

With the fix applied this is ERROR SUMMARY: 0 errors.

Note on scope: valgrind flags only the genuinely-uninitialised reads (the first candidate, before that slot has ever been written). For later candidates the slot holds a stale value that valgrind considers initialised, so it does not report those; the write-count-vs-read-count argument above is what covers that case.

Deterministic confirmation (requires a temporary source edit — diagnostic only, NOT the fix)

To confirm, layout-independently, that the slot is never written, temporarily instrument the call site (edge_drawing.cpp:3254):

// right before ComputeEllipsePoints(circle->eq.coeff, px, py, noPoints):
if (noPoints > 0) { px[noPoints-1] = py[noPoints-1] = -1e300; }   // sentinel
ComputeEllipsePoints(circle->eq.coeff, px, py, noPoints);
// right after it:
if ((noPoints & 1) && px[noPoints-1] == -1e300)
    fprintf(stderr, "odd noPoints=%d: px/py[noPoints-1] left UNWRITTEN\n", noPoints);

For every odd noPoints this fires (e.g. noPoints = 375, 229 on the image above), proving ComputeEllipsePoints never wrote the slot that the validation loop reads. This instrumentation is only for diagnosis and is not part of the proposed fix.

Issue submission checklist

• I report the issue, it's not a question
• I checked the problem with documentation, FAQ, open issues, forum.opencv.org, Stack Overflow, etc and have not found any solution
• I updated to the latest OpenCV version and the issue is still there
• There is reproducer code and related data files