Note on provenance: This issue was found by Claude Opus 5 while reviewing #325. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.
Impact
VectorIndex::appendData tracks the running offset into the target column as a 64 bit value, then writes it to the dataset by casting it down to whatever width the dataset was declared with. If the offset outgrows that width, the cast wraps and a smaller number is written. Both appends report Status::Success.
The result is an index that goes backwards, which is not a valid ragged index. Reading it back through AqNWB throws. Read through h5py or PyNWB the affected row simply comes back empty, because a backwards slice yields nothing, so the data loss is silent on that side.
VectorIndex::initialize explicitly accepts T_U8 and T_U16, so this is a supported configuration, not a misuse.
What happens
Create a VectorIndex with dtype T_U8, append 200 elements, then append 100 more:
appendData(200 elements) Status = Success
appendData(100 elements) Status = Success
Stored index[0] = 200
Stored index[1] = 44 <- static_cast<uint8_t>(300)
True cumulative offset = 300
readIndexedCellValues threw:
VectorIndex::readIndexedCellValues: Invalid index data, currIndex (44) < prevIndex (200)
Nothing failed at write time. The corruption only surfaces on read, and only for readers that check monotonicity.
T_U16 behaves the same way past 65,535 elements.
Cause
The cumulative counter is a uint64_t member, but the write casts it to the dataset width with no range check.
Relevant code
src/nwb/hdmf/table/VectorIndex.cpp:502:
m_currentIndex += elementsAppended;
src/nwb/hdmf/table/VectorIndex.cpp:512-524, the unchecked narrowing:
if (m_dataType.type == IO::BaseDataType::Type::T_U8) {
CellValue indexValue(static_cast<uint8_t>(m_currentIndex));
...
} else if (m_dataType.type == IO::BaseDataType::Type::T_U16) {
...The downstream isCompatibleCellValue check does not help here, because it compares variant alternatives rather than value ranges, so the already truncated value passes.
The read side check that eventually catches it is at :276-282.
Suggested direction
Range check m_currentIndex against the dataset width before the cast and return Status::Failure with a clear message when it will not fit. Widening the dataset automatically, the way HDMF does, would be friendlier but is a much larger change and would alter the on disk dtype mid write.
The same unchecked static_cast<uint32_t> exists on the DynamicTable::addRows path at DynamicTable.cpp:539-541, so a table with more than 4.29 billion ragged elements would wrap there too. Far less likely to be hit, but worth covering in the same fix.
Notes
tests/testVectorIndex.cpp exercises T_U8 but never pushes the offset past 255, so the overflow branch is never reached.
Impact
VectorIndex::appendDatatracks the running offset into the target column as a 64 bit value, then writes it to the dataset by casting it down to whatever width the dataset was declared with. If the offset outgrows that width, the cast wraps and a smaller number is written. Both appends reportStatus::Success.The result is an index that goes backwards, which is not a valid ragged index. Reading it back through AqNWB throws. Read through h5py or PyNWB the affected row simply comes back empty, because a backwards slice yields nothing, so the data loss is silent on that side.
VectorIndex::initializeexplicitly acceptsT_U8andT_U16, so this is a supported configuration, not a misuse.What happens
Create a
VectorIndexwith dtypeT_U8, append 200 elements, then append 100 more:Nothing failed at write time. The corruption only surfaces on read, and only for readers that check monotonicity.
T_U16behaves the same way past 65,535 elements.Cause
The cumulative counter is a
uint64_tmember, but the write casts it to the dataset width with no range check.Relevant code
src/nwb/hdmf/table/VectorIndex.cpp:502:src/nwb/hdmf/table/VectorIndex.cpp:512-524, the unchecked narrowing:The downstream
isCompatibleCellValuecheck does not help here, because it compares variant alternatives rather than value ranges, so the already truncated value passes.The read side check that eventually catches it is at
:276-282.Suggested direction
Range check
m_currentIndexagainst the dataset width before the cast and returnStatus::Failurewith a clear message when it will not fit. Widening the dataset automatically, the way HDMF does, would be friendlier but is a much larger change and would alter the on disk dtype mid write.The same unchecked
static_cast<uint32_t>exists on theDynamicTable::addRowspath atDynamicTable.cpp:539-541, so a table with more than 4.29 billion ragged elements would wrap there too. Far less likely to be hit, but worth covering in the same fix.Notes
tests/testVectorIndex.cppexercisesT_U8but never pushes the offset past 255, so the overflow branch is never reached.