InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

NarrowBandImageFilterBase reads m_RegionList out of bounds when the band has fewer nodes than work units

Open
#6,823 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

`NarrowBandImageFilterBase` indexes `m_RegionList` with the work-unit id, but `NarrowBand::SplitBand`
returns *at most* as many regions as the band has nodes. When the narrow band holds fewer nodes than
work units, every parallel lambda past `m_RegionList.size() - 1` reads a `std::vector` element out of
bounds and iterates from a garbage iterator pair — a segfault, not a diagnosable failure.

## Mechanism

`Initialize()` and `InitializeIteration()` build the region list from the current band:

```cpp
// itkNarrowBandImageFilterBase.hxx:183 and :213
m_RegionList = m_NarrowBand->SplitBand(this->GetNumberOfWorkUnits());
```

`SplitBand` clamps the request down to the band size and emits exactly `t_n` regions:

```cpp
// itkNarrowBand.hxx:37-73
SizeType t_n = n;
const SizeType t_size = m_NodeContainer.size();
if (t_n > t_size)
{
t_n = t_size; // fewer regions than work units; zero when the band is empty
}
...
for (SizeType i = 0; i < t_n; ++i)
{
...
regionList.push_back(region);
}
```

Both parallel loops in `GenerateData()` nevertheless iterate the full work-unit range:

```cpp
// itkNarrowBandImageFilterBase.hxx:103 and :125
mt->ParallelizeArray(
0,
numberOfWorkUnits,
[&](SizeValueType threadId) {
ThreadRegionType splitRegion;
this->GetSplitRegion(threadId, splitRegion);
...
```

and the accessor performs no bounds check:

```cpp
// itkNarrowBandImageFilterBase.hxx:282-286
GetSplitRegion(const size_t & i, ThreadRegionType & splitRegion)
{
splitRegion.first = m_RegionList[i].Begin;
splitRegion.last = m_RegionList[i].End;
}
```

`ThreadedCalculateChange` then dereferences the pair:

```cpp
// itkNarrowBandImageFilterBase.hxx:265-269
for (bandIt = regionToProcess.first; bandIt != regionToProcess.last; ++bandIt)
{
outputIt.SetLocation(bandIt->m_Index);
bandIt->m_Data = df->ComputeUpdate(outputIt, globalData);
}
```

`ThreadedApplyUpdate` walks the same range and writes through it.

## Trigger

The band must hold fewer nodes than work units at any (re)initialization. `m_RegionList` is rebuilt
every time the band is recreated (`InitializeIteration`, `itkNarrowBandImageFilterBase.hxx:207-215`),
so the exposure recurs throughout the evolution, not only at startup.

Band membership is decided by floating-point comparisons against the band width, which is why this
surfaces as a platform-dependent, intermittent crash rather than a reproducible one: a last-bit
difference is enough to move the final node in or out of the band.

`itkNarrowBandThresholdSegmentationLevelSetImageFilterTest` sets `SetNumberOfWorkUnits(2)`, so a band
of 0 or 1 nodes is sufficient. It has been observed to segfault under `ARMBUILD-x86_64-rosetta`
(x86_64 translated onto Apple silicon) while passing on native arm64, Linux, macOS and Windows in the
same run.

An empty band is the worst case: `m_RegionList` is empty and *every* work unit reads out of bounds.

## Secondary defect in the same function

When the band is empty, `t_n` is `0` and `itkNarrowBand.hxx:48` evaluates `0.0f / 0.0f`:

```cpp
auto regionsize = static_cast(std::floor(static_cast(t_size) / static_cast(t_n)));
```

The conversion of the resulting NaN to an unsigned integer type is undefined behavior. The value is
unused because the loop below does not execute, but the guard on the next line (`if (regionsize == 0)`)
is not reliable for a NaN-derived value and the computation should be skipped rather than fixed up.

## Recommended change

Make the region count and the parallel range agree. Two options, in preference order:

1. **Have `SplitBand` honor its argument.** Return exactly `n` regions, padding with empty ranges
(`Begin == End`) once the nodes are exhausted. Every work unit then receives a valid, possibly
empty, range; the threaded bodies already handle an empty range correctly, and no caller needs to
change. Compute `regionsize` only when `t_n > 0`.

2. **Clamp the callers.** Iterate `ParallelizeArray(0, m_RegionList.size(), ...)` at both call sites.
This leaves `SplitBand`'s surprising contract in place for any other caller, so it is the weaker
fix.

In either case `GetSplitRegion` should assert or range-check rather than index blindly, so a future
mismatch fails loudly instead of corrupting memory.

`m_TouchedForThread` is sized to `GetNumberOfWorkUnits()` (`itkNarrowBandImageFilterBase.hxx:193`) and
is indexed over the same range, so it stays correct under either option.

## Note for triage

A proposed fix adding `inputImage->FillBuffer(0.0f)` to the test does not apply. The test's region is
exactly `{64,64,64}` at index `{0,0,0}` and its nested loop assigns every pixel, so the fill is
overwritten in full and cannot change behavior; `seedImage` is likewise fully written by
`evaluate_function`. There is no uninitialized memory in that test.

Contributor guide

Open the contributing guide

Research direction

Start with itkNarrowBand.hxx:37-73 and itkNarrowBandImageFilterBase.hxx:103, 125, 183, 213, and 282-286; inspect SplitBand, GenerateData, and GetSplitRegion together with itkNarrowBandThresholdSegmentationLevelSetImageFilterTest. Run that test with two work units and exercise empty and single-node bands. Done means all work units receive valid ranges, empty bands avoid undefined arithmetic, and the test passes without out-of-bounds access.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-vision, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.