Repository navigation
Conversation
kolkov
left a comment
There was a problem hiding this comment.
Good find. Finite input that ends in a stack overflow is the worst kind of failure, because a caller can't recover from it. Capping recursion depth and emitting the chord matches what the flattening routines already do. Once CI runs, this is the one I'd most like in the next release.
kolkov
left a comment
There was a problem hiding this comment.
The diagnosis is right and I could reproduce the mechanism. At float32 coordinates around 2.3e6 a midpoint rounds back to one of its parents, so addQuad/addCubic recurse on an identical input and never bring the deviation under tolerance. That is a fatal stack overflow, which no caller can recover from — the worst failure class in a library.
On the fix:
- Depth 10 is the right bound. It matches
flattenQuadRecursive,flattenQuadToVelloLinesRecursive,flattenCubicRecursiveandflattenCubicToVelloLinesRecursive(alldepth > 10), so the curve path and the flatten path now stop at the same place. - It cannot truncate a curve that matters. Deviation falls by 4× per subdivision level, so reaching 0.1px tolerance at depth 10 covers a starting deviation of about 100k px — far beyond any sane path. Nothing visible gets faceted by the cap.
- Emitting through
addLineis the right termination. It goes throughclipAndAddLine, so the chord still contributes winding to the visible fill; it does not drop the contour edge.
The test is the strongest part of the change, and I want to say why: a fatal stack overflow cannot be caught, so running each case in a subprocess with debug.SetMaxStack(1<<20) is the only way to assert termination. The matrix (cubic/quad × horizontal/vertical × sign × clipped/unclipped) covers the rounding cases, and TestEdgeBuilderLargeCurvesPreserveClippedFill guards the half that is easy to get wrong — terminating without losing the fill outside the clip.
One observation, and it is pre-existing rather than something this PR introduces: addQuad calls flattenQuadToVelloLines at every level of the subdivision, and that function appends to velloLines unconditionally. I measured it on main: one quadratic with a large deviation produces 352 VelloLines, while a single flatten of the same curve produces 48. Those lines feed AnalyticFillerVello.collectSegments and TileRasterizer.binSegments — the coverage fillers — so the geometry is presented there several times over. It does not affect this fix and I am not asking you to solve it here; I'll open a card for it on our side and reference this PR.
Approving on the code. The CI runs from September need a fresh push before merge; I'd like this one in the next release.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Problem
Large finite coordinates can crash the analytic rasterizer with an unrecoverable stack overflow.
EdgeBuilder.addCubicandaddQuadsubdivide until an absolute 0.1 px deviation test passes, but neither bounds recursion. At float32 precision, a midpoint can round back to an input point and leave a child identical to its parent. The deviation then never decreases.For example, the cubic with x coordinates
{2289292.75, 2289293, 2289293, 2289293}and all y coordinates equal tomath.Float32frombits(0x43817f0a)repeats indefinitely. Its endpoints are one float32 ULP apart. Quadratics have the same failure with x coordinates{2289293, 2289292.75, 2289292.75}.Setting a clip rectangle does not prevent this: the out-of-bounds curve guard runs after the recursive deviation branch.
Fix
Thread a depth through the native quadratic and cubic subdivision helpers. At
depth > 10, emit the remaining chord throughaddLine, matching the existing flattening routines. This keeps line clipping and winding handling on the fallback path and bounds subdivision even when float32 rounding prevents progress.The existing tolerance, subdivision arithmetic and clip-check ordering stay in place. There is no public API change.
Minimal example
This only needs gg; no GPU or window is involved:
On v0.52.5 this ends in
fatal error: stack overflow. With the fix,Fillreturns nil and the clipped blue region covers x=10..99, y=10..89. The standalone reproducer checks all 10,000 pixels: 7,200 filled pixels, with the rest transparent.Tests
TestEdgeBuilderCurveSubdivisionTerminates: 16 subprocess cases cover cubic and quadratic fixed points, both coordinate axes, both signs, and with/without clipping. All 16 fail with stack overflow on unmodified main (bd563f4); all pass with the fix. Each child has a 1 MiB stack limit and a timeout so a regression cannot exhaust the test runner's stack or hang the suite.TestEdgeBuilderLargeCurvesPreserveClippedFill: cubic and quadratic contours extending far beyond the right clip boundary retain the expected visible fill, checked pixel by pixel. These also run in bounded subprocesses.go test -race -tags nogpu ./...passes withCGO_ENABLED=1, following the repository's CI configuration. The raster package was rerun after adding the clipped-fill assertions.go build ./...andgolangci-lint run --timeout=5mpass withCGO_ENABLED=0; the final linter run reports zero issues.Validated on Windows amd64 with Go 1.25.3, based on upstream main at
bd563f4c293fd1f70afe56276c444a38302bac15.