itk::Array/VariableLengthVector SetData: bool-as-size hazard survives PR #6717

[hjmjohnson]

itk::Array::SetData(ptr, false) compiles silently as sz = 0. PR #6717 corrects the two call sites that hit this, but the API hazard that produced them is still present — and itk::VariableLengthVector still ships the ambiguous overload pair that itk::Array dropped in 2016. This issue records the analysis and the recommended follow-up so #6717 can land as-is.

Scope of #6717: correct. Not covered by #6717: the three items under "Recommended paths" below.

Why the bad call compiled, and why it never failed a build

itk::Array declares no two-argument SetData:

void SetDataSameSize(TValue * datain, bool LetArrayManageMemory = false);              // itkArray.h:173
void SetData(TValue * datain, SizeValueType sz, bool LetArrayManageMemory = false);    // itkArray.h:185

SetData(buffer, false) therefore promotes false to SizeValueType 0 and defaults
LetArrayManageMemory to false. bool -> size_t is an integral promotion, not a
narrowing conversion, so no diagnostic is emitted under
-Wall -Wextra -Wconversion -Wsign-conversion.

Measured effect on Array.MemoryManagement:

before #6717: after SetSize(7)          size=7  [ 0 0 0 0 0 0 0 ]
              after SetData(buf,false)  size=0  [ ]
              after Fill(4.0)           size=0  [ ]        <- Fill is a no-op
              buffer[0..6]              uninitialized      <- never written, never read
after  #6717: after Fill(4.0)           size=7  [ 4 4 4 4 4 4 4 ]
              buffer[0..6]                      [ 4 4 4 4 4 4 4 ]

The path is well-defined but vacuous: SetData destroys the 7 owned floats, points at
buffer with size 0 and non-owning, and the destructor's
protected_set_data(nullptr, 0, false) never frees stack memory. No leak, no
out-of-bounds access, no assertion — hence a decade of green CI.

The wrong size was also undetectable by the test's own assertions, which compare a size
against the size it was just copied from:

EXPECT_EQ(test2.GetSize(), notMyOwnBoss.GetSize());   // 0 == 0 passes as readily as 7 == 7
EXPECT_EQ(notMyOwnBoss.GetSize(), myOwnBoss.GetSize());

Three scenarios the test claims to cover — non-owning Array, copy of a non-owning
Array, SetSize with the same size on a non-owning Array — ran on empty arrays.

Origin: the 2016 disambiguation left these two call sites behind

9e49264d330 ("COMP: Disabmiguate function calls to SetData", 2016-03-16) removed
Array::SetData(TValue *, bool) and renamed it SetDataSameSize, because
SizeValueType vs bool could not be disambiguated across 32/64-bit builds with
both internal and external VXL. That commit fixed the one-argument call site it found
(objectToCopy.SetData(data) -> SetDataSameSize(data)) but not the two
SetData(buffer, false) calls, which still compiled — with a silently different
meaning. 19f34ff861c ("ENH: Convert itkArrayTest to GTest") carried them forward
verbatim.

The remaining hazard: VariableLengthVector still carries the overload pair

void SetData(TValue * datain, bool LetArrayManageMemory = false);                       // itkVariableLengthVector.h:718
void SetData(TValue * datain, unsigned int sz, bool LetArrayManageMemory = false);      // itkVariableLengthVector.h:735

Same method name, same module, opposite meaning of the second argument:

call	itk::VariableLengthVector	itk::Array
SetData(p, false)	keeps current size, non-owning	sz = 0
SetData(p, 10)	error: call of overloaded 'SetData(float*&, int)' is ambiguous	sz = 10
SetData(p, intVar)	ambiguous	sz = intVar
SetData(p, sizeTVar)	ambiguous	sz = sizeTVar
SetData(p, sizeTVar, false)	-Wconversion: size_t -> unsigned int may change value	exact

The two-argument integer form on VariableLengthVector is a hard error for every
integer type except unsigned int, which is why no consumer trips it today.

Removing the two-argument overload without a guard re-arms the exact 2016 trap in the
opposite direction: all four previously-ambiguous forms start compiling as sizes, and
any existing SetData(p, true) — legal today, meaning "same size, take ownership" —
would silently become "size 1, non-owning".

Recommended paths (ordered), with sequencing constraints

1. Reject bool as the size argument on itk::Array.

template <typename TSize, std::enable_if_t<std::is_same_v<TSize, bool>, int> = 0>
void
SetData(TValue * datain, TSize sz, bool LetArrayManageMemory = false) = delete;

Verified against the real header:

call	current	with guard
SetData(p, false)	compiles, sz = 0	error: use of deleted function
SetData(p, 10)	compiles	compiles
SetData(p, unsignedVar)	compiles	compiles
SetData(p, intVar, true)	compiles	compiles
SetData(p, sizeTVar)	compiles	compiles

The bool-only deleted template is load-bearing. A non-template
SetData(TValue *, bool, bool = false) = delete makes
SetData(ptr, intVar, true) ambiguous — int -> bool and int -> size_t are both
rank-Conversion — which would break eight downstream call sites that pass an int
size. The constrained template deduces TSize = int, fails the enable_if, and drops
out of the candidate set.

Constraint: this guard requires #6717. With the guard in place,
itkArrayGTest.cxx:81,86 are hard errors, so ITK does not build until those two lines
are corrected.

2. Add VariableLengthVector::SetDataSameSize and deprecate
SetData(TValue *, bool).

Constraint: the guard from item 1 cannot be applied to VariableLengthVector
during a deprecation window — a deleted SetData(TValue *, bool sz, bool = false)
template collides with the legitimate SetData(TValue *, bool LetArrayManageMemory)
it would coexist with, making SetData(p, false) resolve to the deleted overload. The
VariableLengthVector guard must land in the same commit as the removal, not before
it.

Do not widen VariableLengthVector's unsigned int sz to SizeValueType as part
of this. unsigned int is not local to SetData: ElementIdentifier = unsigned int
(line 318) plus every constructor, SetSize, Reserve, and all five reallocation
policies (operator()(unsigned int newSize, unsigned int oldSize)). Changing only
SetData makes the class internally inconsistent; changing all of it reaches
itk::VectorImage and downstream pixel-type machinery, and belongs in a separate
proposal.

3. Replace the tautological assertions in Array.MemoryManagement.

EXPECT_EQ(x.GetSize(), y.GetSize()) after an assignment cannot detect a wrong size.
Assert against the literal element count and check buffer contents. Consider
SetDataSameSize(buffer) in place of SetSize(n) + SetData(buffer, n, false): the
latter allocates n floats and immediately destroys them, which is the allocation the
three-argument overload's documentation says it exists to avoid.

Downstream evidence

Every call site in the open-source ITK consumer set that touches itk::Array or
itk::VariableLengthVector:

Consumer	Site	Call	API
elastix (x8)	Components/Metrics/KNNGraphAlphaMutualInformation/KNN/itkANN{Priority,FixedRadius,Standard}TreeSearch.hxx	ind.SetData(ANNIndices, k, true), int k	Array 3-arg
SimpleITK (x2)	Code/Common/src/sitkPimpleTransform.hxx:96,138	p.SetData(ptr, numberOf*Parameters, false)	Array 3-arg
RTK (x1)	include/rtkSpectralForwardModelImageFilter.hxx:631-632	SetSize(forward.size()); SetData(forward.data_block()); on VariableLengthVector<double>	VLV 2-arg
TubeTK (x1)	src/Numerics/itktubeSingleValuedCostFunctionImageSource.hxx:206	parameters.SetDataSameSize(point.GetDataPointer())	already migrated

No consumer derives from Array or VariableLengthVector; the only subclass is ITK's
own OptimizerParameters. All other downstream SetData matches are unrelated classes
(mitk::DataNode, BlueBerry, Plastimatch's own RANSAC<T>, mdcm::DataEvent,
cmsIT8SetData).

Consequences per recommended path:

• Item 1 (Array guard): zero downstream cost. elastix's int k and SimpleITK's
  size argument both survive; only a literal bool size is rejected.
• Item 2 (VLV removal): breaks RTK at rtkSpectralForwardModelImageFilter.hxx:632
  (error: no matching function for call to 'VariableLengthVector<double>::SetData(double*)'),
  fixed by a one-line change to SetDataSameSize. This is why item 2 must ship as a
  deprecation first, with removal deferred until RTK has migrated.
• Item 3 (test assertions): ITK-internal test source; no downstream reach.

ITK-internal call sites all pass a genuine integer size and are unaffected by item 1:
itkLBFGS2Optimizerv4.hxx:146,150, itkOptimizerParametersHelper.h:60,
itkImageVectorOptimizerParametersHelper.hxx:76, itkMetaArrayReader.h:130,209,
itkImageToImageMetricv4GetValueAndDerivativeThreaderBase.hxx:73,
itkMultiResolutionPDEDeformableRegistrationTest.cxx:250,262.