InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
itk::Array/VariableLengthVector SetData: bool-as-size hazard survives PR #6717
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
`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`:
```cpp
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:
```cpp
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
```cpp
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`.**
```cpp
template , 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` | **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`, `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::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`.
Contributor guide
Research direction
Start with itkArray.h, itkVariableLengthVector.h, and itkArrayGTest.cxx:81,86, then review the documented overloads, call sites, and sequencing constraints. Check the relevant tests and compile probes before making changes; done means the bool-size hazard is rejected, VariableLengthVector migration is coordinated with RTK, and tautological assertions validate literal sizes and buffer contents.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design, testing-qa
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100