Validate dx/dy/dz spacing arguments across all C++ operator constructors (H2)#425
Open
gpagallo wants to merge 3 commits into
Open
Validate dx/dy/dz spacing arguments across all C++ operator constructors (H2)#425gpagallo wants to merge 3 commits into
gpagallo wants to merge 3 commits into
Conversation
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this? (check all applicable)
Description
Adds input validation for the cell-spacing arguments (
dx,dy,dz)across every C++ operator constructor and
AddScalarBCfree function. Eachspacing is now required to be strictly positive and finite;
violations throw
std::invalid_argumentwith a descriptive message namingthe offending parameter and value.
Previously these values were accepted unchecked. Passing
dx = 0producedsilent
Inf/NaNpropagation through the assembled operator: the 1-Dnon-periodic Gradient and Divergence constructors both end with
*this /= dx;(gradient.cpp:298,divergence.cpp:262) and the staticperiodicGrad1D/periodicDiv1Dhelpers used by the periodic-BCconstructors 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: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:
src/cpp/gradient.cpp(+gradient.h)periodicGrad1Dsrc/cpp/divergence.cpp(+divergence.h)src/cpp/laplacian.cpp(+laplacian.h)src/cpp/robinbc.cpp(+robinbc.h)src/cpp/mixedbc.cpp(+mixedbc.h)src/cpp/addscalarbc.cpp(+addscalarbc.h)addScalarBClhs×3,addScalarBC×3src/cpp/utils.cpp(+utils.h)mole::check_spacinghelperThe four typed Interpol classes (
InterpolCtoF,InterpolCtoN,InterpolFtoC,InterpolNtoC) and the originalInterpolare unaffected —they take no spacing argument.
Backward compatibility
dx = 0,negative spacing,
NaNor±Infand produced corrupted operators will nowthrow
std::invalid_argumentinstead. This is the intended fix; affectedcallers were already producing incorrect results silently.
-->
QA Instructions, Screenshots, Recordings
Added/updated tests?
have not been included
A new GoogleTest file
tests/cpp/test_spacing_validation.cppis added withthe following cases:
SpacingValidation.GradientRejectsZeroGradient(2, 10, 0.0)throwsstd::invalid_argumentSpacingValidation.GradientRejectsNegativeGradient(2, 10, -1.0)throwsSpacingValidation.GradientRejectsNaNGradient(2, 10, NaN)throwsSpacingValidation.GradientRejectsInfGradient(2, 10, Inf)throwsSpacingValidation.DivergenceRejectsZeroDivergenceSpacingValidation.LaplacianRejectsZeroDzdz = 0throwsSpacingValidation.RobinBCRejectsZeroRobinBCwithdx = 0throwsSpacingValidation.MixedBCRejectsNegativeMixedBCwithdx = -0.1throwsSpacingValidation.AddScalarBCRejectsZeroaddScalarBCfree function withdx = 0throwsSpacingValidation.GradientAcceptsValidSpacingGradient(2, 10, 0.1)does not throwTests 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
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:
Inf/NaN) is worth aone-line entry in the release notes for the next minor version.
pathological spacings should be updated to expect the exception.
[optional] What gif best describes this PR or how it makes you feel?