Skip to content

Validate dx/dy/dz spacing arguments across all C++ operator constructors (H2)#425

Open
gpagallo wants to merge 3 commits into
csrc-sdsu:mainfrom
gpagallo:fix/h2-spacing-validation
Open

Validate dx/dy/dz spacing arguments across all C++ operator constructors (H2)#425
gpagallo wants to merge 3 commits into
csrc-sdsu:mainfrom
gpagallo:fix/h2-spacing-validation

Conversation

@gpagallo

@gpagallo gpagallo commented Jul 7, 2026

Copy link
Copy Markdown

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Example
  • Documentation

Description

Adds input validation for the cell-spacing arguments (dx, dy, dz)
across every C++ operator constructor and AddScalarBC free function. Each
spacing is now required to be strictly positive and finite;
violations throw std::invalid_argument with a descriptive message naming
the offending parameter and value.

Previously these values were accepted unchecked. Passing dx = 0 produced
silent Inf/NaN propagation through the assembled operator: the 1-D
non-periodic Gradient and Divergence constructors both end with
*this /= dx; (gradient.cpp:298, divergence.cpp:262) and the static
periodicGrad1D / periodicDiv1D helpers used by the periodic-BC
constructors also divide by dx (gradient.cpp:91, divergence.cpp:93).
Passing a negative spacing yielded a sign-flipped operator with no warning.
This corresponds to finding H2 in the most recent codebase security
audit.

What changed

A single helper is introduced in src/cpp/utils.h / utils.cpp:

// utils.h
namespace mole {
    void check_spacing(Real h, const char* name);
}
// utils.cpp
void mole::check_spacing(Real h, const char* name) {
    if (!std::isfinite(h) || h <= 0.0) {
        throw std::invalid_argument(
            std::string("MOLE: ") + name +
            " must be a positive finite number, got " + std::to_string(h));
    }
}

The helper is then called at the top of every entry point that takes a
spacing argument — not only at leaf constructors — so a caller always sees
an error pointing at the function they actually invoked, with the correct
parameter name.

Files touched

Six source pairs are modified, covering 29 entry points:

File Entry points validated
src/cpp/gradient.cpp (+ gradient.h) 6 ctors (1-D, 2-D, 3-D × {non-periodic, periodic}) + periodicGrad1D
src/cpp/divergence.cpp (+ divergence.h) same 7 entry points as Gradient
src/cpp/laplacian.cpp (+ laplacian.h) 1-D, 2-D, 3-D ctors
src/cpp/robinbc.cpp (+ robinbc.h) 1-D, 2-D, 3-D ctors
src/cpp/mixedbc.cpp (+ mixedbc.h) 1-D, 2-D, 3-D ctors
src/cpp/addscalarbc.cpp (+ addscalarbc.h) addScalarBClhs ×3, addScalarBC ×3
src/cpp/utils.cpp (+ utils.h) New mole::check_spacing helper

The four typed Interpol classes (InterpolCtoF, InterpolCtoN,
InterpolFtoC, InterpolNtoC) and the original Interpol are unaffected —
they take no spacing argument.

Backward compatibility

  • Source-compatible. No public signatures change.
  • ABI-compatible. No member layout changes.
  • Behavioral change. Code paths that previously accepted dx = 0,
    negative spacing, NaN or ±Inf and produced corrupted operators will now
    throw std::invalid_argument instead. This is the intended fix; affected
    callers were already producing incorrect results silently.

-->

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

A new GoogleTest file tests/cpp/test_spacing_validation.cpp is added with
the following cases:

Test Assertion
SpacingValidation.GradientRejectsZero Gradient(2, 10, 0.0) throws std::invalid_argument
SpacingValidation.GradientRejectsNegative Gradient(2, 10, -1.0) throws
SpacingValidation.GradientRejectsNaN Gradient(2, 10, NaN) throws
SpacingValidation.GradientRejectsInf Gradient(2, 10, Inf) throws
SpacingValidation.DivergenceRejectsZero analogous for Divergence
SpacingValidation.LaplacianRejectsZeroDz 3-D ctor with dz = 0 throws
SpacingValidation.RobinBCRejectsZero 1-D RobinBC with dx = 0 throws
SpacingValidation.MixedBCRejectsNegative 1-D MixedBC with dx = -0.1 throws
SpacingValidation.AddScalarBCRejectsZero addScalarBC free function with dx = 0 throws
SpacingValidation.GradientAcceptsValidSpacing sanity check — Gradient(2, 10, 0.1) does not throw

Tests cover each impacted entry-point family (Gradient, Divergence,
Laplacian, RobinBC, MixedBC, AddScalarBC) and each invalid value class
(zero, negative, NaN, Inf), plus a positive sanity case.

Keep-open request

  • I am requesting maintainer review for keep-open.
    Reason:

Read Contributing Guide and Code of Conduct

[optional] Are there any post deployment tasks we need to perform?

None. Two minor follow-ups are worth noting but are not blocking:

  1. The behavioral change (throw instead of silent Inf/NaN) is worth a
    one-line entry in the release notes for the next minor version.
  2. Any downstream examples or tutorials that intentionally exercised
    pathological spacings should be updated to expect the exception.

[optional] What gif best describes this PR or how it makes you feel?

gpagallo added 3 commits July 6, 2026 17:08
 Introduces a small helper that rejects zero, negative, NaN and Inf
spacing arguments with std::invalid_argument. Used by every operator
constructor and AddScalarBC free function that takes a dx/dy/dz
parameter, so validation is active in both Debug and Release builds
(unlike assert(), which vanishes under NDEBUG). Matches the existing
throw std::invalid_argument pattern already used in MixedBC.
Adds mole::check_spacing calls at the top of every C++ operator
constructor and AddScalarBC function that takes a spacing argument
(29 entry points across 6 source files, including the periodicGrad1D
and periodicDiv1D static helpers). Each entry point is validated
individually so error messages point at the function the caller
actually invoked. Closes finding H2 from the June 2026 codebase
security audit.
Covers Gradient, Divergence, Laplacian, RobinBC, MixedBC and
AddScalarBC with zero, negative, NaN and Inf inputs, plus positive
sanity checks and a check that the error message names the
offending parameter.
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

Operator constructors silently accept zero, negative, NaN and Inf for dx/dy/dz

1 participant