Avoid NULL deref in KillFace() from tessMeshConnect().

[wrmorris-google]

This avoids a NULL deref in KllFace() that occurs when NULL is passed as the face to delete.

[wrmorris-google]

I don't understand the flow of the algorithm well enough to be able to eyeball it - but might it be ok to instead just guard the call to KillFace() against passing a nULL instead of returning NULL here?

[memononen]

Your PR changes tessMeshConnect() but you talk about KillFace(). You will need to elaborate what you're doing :)

[wrmorris-google]

In tessMeshConnect() line 499:

		KillFace( mesh, eDst->Lface, eOrg->Lface );

eDst->Lface can be NULL (I've traced a crash to here). We need to make sure that this NULL is not passed to KillFace(), because it'll crash.

KillFace() itself cannot return an error code, so I don't know a better way to manage this - I'm assuming that the call to KillFace() is necessary to maintain the integrity of the data structure as a whole.

I don't understand the operation of the algorithm well enough to be certain one way or other, but I wonder if potentially just guarding against passing a NULL to KillFace() would suffice?

For example, in tessMeshConnect():

...
	if( eDst->Lface != eOrg->Lface ) {
		/* We are connecting two disjoint loops -- destroy eDst->Lface */
		joiningLoops = TRUE;
		if (eDst->Lface != NULL)
			KillFace( mesh, eDst->Lface, eOrg->Lface );
	}
...

I hesitate to suggest this, however, because I'm not sure if this situation (eDst->Lface == NULL) is something rare but safe in this case, or if it itself is an indication of a corrupted dataset that should be aborted.

[memononen]

I think we're in "this should never happen" territory, and I suspect something else went wrong earlier.

Do you have more info what leads into the crash?

One feature of the tessellator is that it creates sentinel edges around the data. All other edges should have valid faces, except the far sides of the sentinel edges.

My best guess is that you have a case where the input has NaNs or very big numbers which messes up the sentinel stuff.

[memononen]

Are you working together with @sfreilich ? He's currently looking into the too-big-coords stuff: #71

[sfreilich]

We're on different teams, but yes. Though maybe should be coordinating at a bit more fine-grained level. We're both working on fuzz-tests that are slightly downstream, there's presumably a lot of overlap.

(Looking forward to your take on my latest attempt at that other PR, though I'm on vacation until Thursday.)

[wrmorris-google]

As @sfreilich mentions, we're on different teams that both use libtess2.

My team doesn't have proper fuzz tests for our libtess2 usage at the moment - but we do have a massive dataset that spits out crash reports from time to time when we process it :)

Let me see if I can better isolate the inputs that are involved with this crash specifically.

[wrmorris-google]

I've managed to produce a minimal input set that produces this error, which is an assert failure in sweep.c:AddRightEdges()

assert( VertLeq( e->Org, e->Dst ));

with call stack:
sweep.c:AddRightEdges
sweep.c:SweepEvent
sweep.c:ComputeInterior
tess.c:tessTesselate

With the following data failing the assert:

e->Org->s == -2.66350293
e->Org->t ==  2.52593327
e->Dst->s == -2.71672201
e->Dst->t ==  2.47274876

The test case that produces this assertion failure:

  TESStesselator* libtess = tessNewTess(nullptr);
  const int kSize = 2;
  const int kStride = kSize * sizeof(float);
  float contour_1[] = {
      0.6451516f,  -0.78484166f,  //
      0.68063074f, -0.74938494f,  //
      -2.628021f,  2.5613577f,    //
      -2.6635f,    2.5259008f,    //
  };
  tessSetOption(libtess, TESS_REVERSE_CONTOURS, 0);
  tessAddContour(libtess, kSize, contour_1, kStride,
                 sizeof(contour_1) / sizeof(float) / 2);

  float contour_2[] = {
      -3.5651314f, 3.428146f,   //
      -3.6183505f, 3.3749611f,  //
      -2.716722f,  2.4727488f,  //
      -2.663503f,  2.5259333f,  //
  };
  tessSetOption(libtess, TESS_REVERSE_CONTOURS, 1);
  tessAddContour(libtess, kSize, contour_2, kStride,
                 sizeof(contour_2) / sizeof(float) / 2);

  float contour_3[] = {
      -3.5651336f, 3.4281437f,  //
      -3.6183527f, 3.374959f,   //
      -2.7167032f, 2.47273f,    //
      -2.6634843f, 2.5259147f,  //
  };
  tessSetOption(libtess, TESS_REVERSE_CONTOURS, 1);
  tessAddContour(libtess, kSize, contour_3, kStride,
                 sizeof(contour_3) / sizeof(float) / 2);

  float contour_4[] = {
      -3.5651345f, 3.4281447f,  //
      -3.6183536f, 3.37496f,    //
      -1.3004229f, 1.0555387f,  //
      -1.247204f,  1.1087234f,  //
  };
  tessSetOption(libtess, TESS_REVERSE_CONTOURS, 1);
  tessAddContour(libtess, kSize, contour_4, kStride,
                 sizeof(contour_4) / sizeof(float) / 2);

  float normal[] = {0.0f, 0.0f, 1.0f};
  tessTesselate(libtess, TESS_WINDING_POSITIVE, TESS_POLYGONS,
                /*polySize=*/3, /*vertexSize=*/2, normal);
  tessDeleteTess(libtess);

I've not looked into this further, and may not be able to for at least the next few days because of other commitments. Any ideas?

[memononen]

There's collinear edges in the input, which is likely the culprit.

Smells something very similar to: #37

Can you test if reverting the CL fixes things?

I think we should revert that CL, and see if there's alternative fix for the original issue.

[wrmorris-google]

If you mean reverting 955761f (the fix to #22) - then yes, getting rid of that appears to avoid this crash.

[memononen]

Correct. Good to know, I think we should revert that CL then as a fix. #22 may require different kind of fix then.

[wrmorris-google]

#73 is a PR for reverting that fix for #22. If you'd like, I can close this PR now in preference for #73.

[wrmorris-google]

Closing in favor of #73.

[memononen]

Thanks! I merged the fix, and reopened #22.