Skip to content

Conversation

sfreilich
Copy link
Collaborator

This avoids an assert-fail crash in debug mode, and reads of uninitialized memory and possibly crashes in optimized builds.

@sfreilich sfreilich requested a review from memononen April 16, 2025 20:31
@sfreilich
Copy link
Collaborator Author

I still don't understand exactly why this constraint is getting violated, though. But fuzztester seems to find a lot of polylines that violate it, even without getting stuck in NaN-land.

@memononen
Copy link
Owner

I think it is unrealistic that the algorithm is able to deal with max float. The issue with the large floats are that they quickly turn into infs, and then down the line either 0 or inf depending how they are used. The brittleness is in tesedgeIntersect().

To get to the bottom of the issue, I think you should check what the s/t values are after projection, as well as what the sentinel edges are InitEdgeDict().

One solution could be to clamp the input points to a safe range. We should be able to figure out the sane range from tesedgeIntersect().

@sfreilich
Copy link
Collaborator Author

For this particular example, the edge that violated that assert had:

e->Org->s,t: 1.000000,0.000000, e->Dst->s,t: 0.594775,0.000000

That is, Org->s > Dst->s and it's not looking at t.

You're right that those sentinel edges are overflowing. The projection vectors for those are:

e->Org->s,t: inf,-2.010000, e->Dst->s,t: -340282346638528859811704183484516925440.000000,-2.010000
e->Org->s,t: inf,1.010000, e->Dst->s,t: -340282346638528859811704183484516925440.000000,1.010000

The code in InitEdgeDict which handles this is:

w = (tess->bmax[0] - tess->bmin[0]) + (TESSreal)0.01;
h = (tess->bmax[1] - tess->bmin[1]) + (TESSreal)0.01;

smin = tess->bmin[0] - w;
smax = tess->bmax[0] + w;
tmin = tess->bmin[1] - h;
tmax = tess->bmax[1] + h;

That seems equivalent to:

smin = 2 * tess->bmin[0] - tess->bmax[0] - (TESSreal)0.01 ;
smax = 2 * tess->bmax[0] - tess->bmin[0] + (TESSreal)0.01;

Are you suggesting clamping the values generated for the projection vectors in tessProjectPolygon? (Clamping to [(FLOAT_MIN + 0.01) / 2.0, (FLOAT_MAX - 0.01) / 2 .0]???) Or are you suggesting clamping the input coords somewhere in tesedgeIntersect? I looked at that, but I'm a bit lost?

@sfreilich
Copy link
Collaborator Author

s is the dot-product of coords and (0, 1, 0). t is the dot-product of coords and (0, 0, 1) or (0, 0, -1). So that implies the same safe bounds for coords as for s and t. I think.

@sfreilich
Copy link
Collaborator Author

And I'd also guess that it might be better to fail in tessAddContour rather than modify values that fall out of that range.

@sfreilich
Copy link
Collaborator Author

Made an attempt at this. It changes some of the previous test-cases to exit with error due to invalid input.

Source/tess.c Outdated
const TESSreal* coords = (const TESSreal*)src;
src += stride;
if (isnan(coords[0]) || isnan(coords[1]) || (size > 2 && isnan(coords[2]))) {
if (isnan(coords[0]) ||
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the per component check a helper function.

@memononen
Copy link
Owner

I had bunch of pending comments from the other day, and did not realize that, sorry :)

@sfreilich sfreilich force-pushed the segfault branch 2 times, most recently from 1f1bf71 to fb0caef Compare April 24, 2025 14:14
@sfreilich
Copy link
Collaborator Author

Updated, please take a look.

Copy link
Owner

@memononen memononen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nags, should be trivial to fix. Other than that the implementation looks good.

The computation of sentinel edges in InitEdgeDict potentially overflows
if coordinates are more than half of float max/min, and it distinguishes
between real coord bounds and those sentinel edges by a 0.01 margin,
which is not distinguishable at that magnitude due to loss-of-precision.
That can cause assert failures in debug mode or uninitialized reads or
crashes in optimized mode.

Instead, fail if coordinates are not in `[-2^32, 2^32]`. This defines
TESS_MAX_VALUE and TESS_MIN_VALUE in the public headers as those bounds,
in case clients want to clamp or otherwise sanitize invalid inputs.
@sfreilich sfreilich merged commit c3b8ed4 into memononen:master Apr 24, 2025
1 check passed
@sfreilich sfreilich deleted the segfault branch April 24, 2025 21:26
@sfreilich sfreilich mentioned this pull request Apr 24, 2025
@sfreilich sfreilich changed the title Upgrade an assert to explicit error-handling in AddRightEdges Fail if coords are outside of a safe range Apr 25, 2025
@sfreilich
Copy link
Collaborator Author

I messed up the range in the change description, the bounds are +/- 2^23, not 2^32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants