InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

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

Open
#6,718 0 comments 0 reactions 0 assignees View on GitHub
area:Core type:Design
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.