Skip to content

fix(datasource): degrade to empty on shape lies instead of Q_ASSERT-only validation#31

Merged
jeandet merged 2 commits into
SciQLop:mainfrom
jeandet:fix/datasource-shape-validation
Jun 12, 2026
Merged

fix(datasource): degrade to empty on shape lies instead of Q_ASSERT-only validation#31
jeandet merged 2 commits into
SciQLop:mainfrom
jeandet:fix/datasource-shape-validation

Conversation

@jeandet

@jeandet jeandet commented Jun 12, 2026

Copy link
Copy Markdown
Member

The SoA/multi/row-major/2D data source constructors validated caller shapes with Q_ASSERT alone: compiled out in release builds the lies became out-of-bounds reads (e.g. mYSize derived from a non-multiple nz), and in debug builds they aborted the host application. These are public entry points fed with arbitrary user data by downstream consumers (SciQLopPlots).

Constructors now check the same invariants unconditionally and degrade to an empty source with a qWarning when violated; genuinely empty input stays silently empty. Q_ASSERT remains in the hot index accessors.

Tests (all aborted via QFATAL ASSERT before the fix):

  • soaMismatchedLengthsDegradeToEmpty
  • soaMismatchedColumnDegradesToEmpty
  • rowMajorInvalidShapeDegradesToEmpty
  • soa2dInvalidShapesDegradeToEmpty

Part of the 2026-06-09 audit (H8 in SciQLopPlots docs/backlog-2026-06-10.md); companion to the SciQLopPlots set_data-hardening PR.

🤖 Generated with Claude Code

…nly validation

The SoA/multi/row-major/2D data source constructors validated caller
shapes with Q_ASSERT alone: compiled out in release builds the lies
became out-of-bounds reads (e.g. mYSize derived from a non-multiple nz),
and in debug builds they aborted the host application. These are public
entry points fed with arbitrary user data by downstream consumers.

Constructors now check the same invariants unconditionally and degrade
to an empty source with a qWarning when violated; genuinely empty input
stays silently empty. Q_ASSERT remains in the hot index accessors.

Tests: soaMismatchedLengthsDegradeToEmpty, soaMismatchedColumnDegradesToEmpty,
rowMajorInvalidShapeDegradesToEmpty, soa2dInvalidShapesDegradeToEmpty —
all aborted via QFATAL ASSERT before the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens several public data source constructors against inconsistent caller-provided shapes by replacing debug-only Q_ASSERT shape validation with unconditional runtime checks that “degrade to empty” (and warn) rather than risking out-of-bounds reads in release builds or aborts in debug builds.

Changes:

  • Add unconditional shape validation + qWarning-and-empty fallback in SoA (1D), SoA multi-column, SoA 2D, and row-major multi data source constructors.
  • Add unit tests that exercise invalid-shape inputs and verify sources degrade to empty (instead of QFATAL/asserting).
  • Keep Q_ASSERT in hot-path accessors while moving “public entry point” validation into constructors.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/auto/test-multi-datasource/test-multi-datasource.h Adds new shape-validation test declarations for multi datasources.
tests/auto/test-multi-datasource/test-multi-datasource.cpp Adds new tests for SoA-multi and row-major invalid shape handling.
tests/auto/test-datasource2d/test-datasource2d.h Adds invalid-shape test declaration for 2D datasource.
tests/auto/test-datasource2d/test-datasource2d.cpp Adds invalid-shape tests for SoA 2D datasource degradation behavior.
tests/auto/test-datasource/test-datasource.h Adds mismatched-lengths degradation test declaration for SoA 1D datasource.
tests/auto/test-datasource/test-datasource.cpp Adds mismatched-lengths degradation test for SoA 1D datasource.
src/datasource/soa-multi-datasource.h Replaces Q_ASSERT-only column length validation with runtime checks that drop data on mismatch.
src/datasource/soa-datasource.h Replaces Q_ASSERT-only key/value length validation with runtime checks that drop data on mismatch.
src/datasource/soa-datasource-2d.h Adds runtime validation for nx/ny/nz consistency; drops data (and warns) on invalid shapes.
src/datasource/row-major-multi-datasource.h Adds runtime shape validation; drops data (and warns) on invalid shapes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +84 to +92
const bool valid = rows > 0 && columns >= 0 && stride > 0 && stride >= columns
&& static_cast<int>(keys.size()) == rows && values != nullptr;
if (!valid)
{
if (rows != 0)
qWarning("QCPRowMajorMultiDataSource: invalid shape (rows=%d, columns=%d, "
"stride=%d, keys=%zu, values=%p) — dropping data",
rows, columns, stride, keys.size(),
static_cast<const void*>(values));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cfade18 — the gate is now rows != 0 || !keys.empty(), matching the 2D source's any-input-non-empty convention. Genuinely empty input (rows == 0, no keys) stays silent. Reproducer added to rowMajorInvalidShapeDegradesToEmpty (QTest fails when the expected warning isn't emitted).

The row-major ctor's diagnostic was gated on rows != 0, so rows == 0 with
non-empty keys dropped data silently — inconsistent with the other
mismatch paths and with the 2D source's any-input-non-empty gate.
Genuinely empty input (rows == 0, no keys) stays silent.

Review follow-up on PR SciQLop#31.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jeandet jeandet merged commit fbd76e0 into SciQLop:main Jun 12, 2026
5 checks passed
@jeandet jeandet deleted the fix/datasource-shape-validation branch June 12, 2026 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants