[23_29] fix: ellipse_rep::get_control_points abs array length mismatch#3048
Merged
da-liii merged 1 commit intoMoganLab:mainfrom Mar 29, 2026
Merged
Conversation
Make abs array have one entry per control point to match the return value, consistent with all other get_control_points implementations. Previously abs had 2 elements but np=3, causing out-of-bounds access in curve_box_rep::graphical_select which led to infinite loops when drawing ellipses.
|
Thanks for the explanation; I'll definitely be focusing on hunting down the root cause for future bugs! Closing my PR in favor of this one. |
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
ellipse_rep::get_control_points returns abs with 2 elements {0.0, 1.0} but np = N(points) = 3. In curve_box_rep::graphical_select, abs is indexed up to abs[np - 1], i.e. abs[2], which is out of bounds. The garbage value read from memory is passed as t2 to find_closest_point, causing curvet_closest_points to loop indefinitely — this is the hang when drawing ellipses.
Fix
Make abs have one entry per control point, restoring the invariant N(abs) == np that every other get_control_points implementation (segment, poly_segment, spline, arc, poly_bezier) maintains. The fix is in the source (ellipse_rep) rather than at the call site.
The values are computed by evenly splitting the parameter range [0, 1] across n control points. For the ellipse case (n=3), this produces {0.0, 0.5, 1.0}. The specific values don't affect correctness — they only determine how the parameter range is partitioned for the closest-point search. Any partition covering [0, 1] yields the same result.
Regarding #3047
PR #3047 clamps np to N(abs) in graphical_select:
if (np > N(abs)) {
np = N(abs);
}
This prevents the crash, but has a few concerns:
Fixes the symptom, not the cause — ellipse_rep::get_control_points still violates the N(abs) == np contract. Any other caller of get_control_points that trusts the return value would hit the same bug.
Inconsistent state after clamping — np becomes 2 but pts still has 3 elements. The first loop iterates over N(pts) = 3 control point handles, but the second loop uses np = 2 for segment search, so the data structures disagree.
graphical_distance is not protected — the other call site at line 423 also uses np from get_control_points to index pts, and is not patched by #3047.