PointSet::SetPoints(PointsVectorContainer *) overload leads to undefined behavior

[N-Dekker]

Description

PointSet::SetPoints(PointsVectorContainer * points) internally casts its argument to a PointsContainer *:

ITK/Modules/Core/Common/include/itkPointSet.hxx

Line 76 in 9a5dade

auto * pointsPtr = reinterpret_cast<PointsContainer *>(points);

This leads to undefined behavior, when the PointsVectorContainer object is being used as a PointsContainer object.

Steps to Reproduce

The test code already has undefined behavior, even if it might just work on the currently tested platforms:

ITK/Modules/Core/Common/test/itkPointSetTest.cxx

Lines 102 to 127 in 9a5dade

	// Insert the points using SetPointArray.
	PointsVectorContainerPointer pointsArray = PointsVectorContainer::New();
	// Test for in-compatible input array dimension
	pointsArray->InsertElement(0, 1.0);
	ITK_TRY_EXPECT_EXCEPTION(pset->SetPoints(pointsArray));
	pointsArray->Reserve(numOfPoints * pointDimension);
	int index = 0;
	for (int i = 0; i < numOfPoints; ++i)
	{
	pointsArray->SetElement(index++, 1.0);
	pointsArray->SetElement(index++, 2.0);
	pointsArray->SetElement(index++, 3.0);
	}
	pset->SetPoints(pointsArray);
	// Check the count of points after insertion using SetPoints.
	pointsArrayAfter = pset->GetPoints();
	if (static_cast<int>(pointsArrayAfter->Size()) != numOfPoints)
	{
	std::cerr << "Mismatch in count after inserting points." << std::endl;
	return EXIT_FAILURE;
	}

Versions

• The PointSet::SetPoints(PointsVectorContainer *) overload is introduced by pull request ENH: Support for Mesh serialization #3154 commit 6827360 "ENH: Adding SetPoints in PointSet for 1D input array", merged on Mar 16, 2022, and included with both v5.3.0 and v5.4.0.

Additional Information

Discussed before at #3154 (comment), with @PranjalSahu

[ebrahimebrahim]

Note that the problem has moved to here since #4867:

ITK/Modules/Core/Common/include/itkPointSetBase.hxx

Lines 72 to 73 in ec75692

	// Note: this cast is unsafe. It may lead to undefined behavior.
	auto * pointsPtr = reinterpret_cast<PointsContainer *>(points);

[N-Dekker]

Would it be OK if we would just deprecate, and eventually remove this particular SetPoints(PointsVectorContainer *) overload? @PranjalSahu What do you think?

[PranjalSahu]

Yes, it can be deprecated. But for the future, we should think of a way for Python to allow direct point setting without creating a new copy. Because there are use cases where the meshes can be in GB size and for performing some simple filtering, a new copy would be too much.