From fa8b4a88f4be68cf57ef3b194d45eee66144ea24 Mon Sep 17 00:00:00 2001 From: petlenz Date: Sun, 20 Sep 2026 10:49:18 +0200 Subject: [PATCH] core: reject malformed models instead of running them Four checks that a model could previously fail silently, found by a repo-wide review. Each is reproduced by a test that fails when the check is reverted. Duplicate material names (#56). Names are what every input wire, material reference and property lookup resolves against, and a second material under a live name simply overwrote the first in the name index and the material handler. Consumers wired before the collision kept the first, consumers wired after got the second. With mismatched property types that is a wrong pointer rather than a wrong answer -- lookups resolve through a static_cast, so a tensor consumer writes through a pointer to a double, which ASan reports as a heap-buffer-overflow. Checked before construction, because a material registers its properties while constructing: by the time adopt() runs, the collision has already happened in the registry. tensor_component_stepper "indices" (#57). The parameter addresses a tensor component directly and was passed to the subscript unchecked, so [9,9] on a 3x3 read and wrote past the end of the tensor, and a one-element list read past the end of the vector itself. tmech checks neither. Validated once in the constructor rather than in update(), which runs per increment. update_property() with a name that is not in the graph (#69). It returned quietly, which made a typo indistinguishable from a property that legitimately never changes: the driver asks for an update, gets no error, and reads a stale value for the rest of the analysis. commit()/revert() before finalize() (#60). Their siblings all call check_finalized; these two did not, and the engine's history list is built by finalize(), so they walked an empty list and reported success. Also makes object_store::find() honest about noexcept (#64): it built a std::string for the lookup key, and a bad_alloc under noexcept is std::terminate. A transparent hash lets it hash the string_view directly, which also removes the allocation from every lookup. --- .../numsim-materials/core/material_context.h | 15 +- include/numsim-materials/core/object_store.h | 47 +++- .../numsim-materials/core/property_engine.h | 9 +- .../materials/tensor_component_stepper.h | 30 ++- tests/CMakeLists.txt | 1 + tests/test_model_validation.cpp | 248 ++++++++++++++++++ 6 files changed, 344 insertions(+), 6 deletions(-) create mode 100644 tests/test_model_validation.cpp diff --git a/include/numsim-materials/core/material_context.h b/include/numsim-materials/core/material_context.h index 2b57391..ba336bd 100644 --- a/include/numsim-materials/core/material_context.h +++ b/include/numsim-materials/core/material_context.h @@ -86,8 +86,19 @@ class material_context { m_engine.update_property(material, property, exclude); } - void commit() { m_engine.commit(); } - void revert() { m_engine.revert(); } + /// Advance / roll back history. The driver owns these calls -- materials and + /// solvers never make them. Both need the same finalize() gate their siblings + /// have: the engine's history list is built by finalize(), so on an + /// unfinalized context they used to be silent no-ops rather than errors. + void commit() { + check_finalized("commit"); + m_engine.commit(); + } + + void revert() { + check_finalized("revert"); + m_engine.revert(); + } // --- Checkpoint / Restart --- diff --git a/include/numsim-materials/core/object_store.h b/include/numsim-materials/core/object_store.h index df64280..bd8311a 100644 --- a/include/numsim-materials/core/object_store.h +++ b/include/numsim-materials/core/object_store.h @@ -1,9 +1,11 @@ #ifndef OBJECT_STORE_H #define OBJECT_STORE_H +#include #include #include #include +#include #include #include #include "numsim-materials/core/material_base.h" @@ -36,6 +38,7 @@ class object_store { template Material& create(parameter_handler& params, property_handler& properties, material_handler& materials) { + check_name_available(params); auto ptr = std::make_unique(params, properties, materials); auto& ref = *ptr; adopt(std::move(ptr), materials); @@ -47,6 +50,7 @@ class object_store { parameter_handler& params, property_handler& properties, material_handler& materials) { + check_name_available(params); auto ptr = factory_type::instance().create( type_name, params, properties, materials); auto& ref = *ptr; @@ -55,8 +59,12 @@ class object_store { } /// Find by name. + /// + /// Transparent lookup: hashing a string_view directly keeps this allocation + /// free, which is what lets it be noexcept. Building a std::string for the + /// key could throw bad_alloc, and under noexcept that is std::terminate. material_interface_type* find(std::string_view name) const noexcept { - auto it = m_by_name.find(std::string(name)); + auto it = m_by_name.find(name); return it != m_by_name.end() ? it->second : nullptr; } @@ -65,6 +73,32 @@ class object_store { } private: + /// Reject a duplicate name before anything is constructed. + /// + /// Names are the only way materials address each other -- every input wire, + /// every material_ref and every property lookup resolves against one. A + /// second material under a live name used to overwrite the first in the name + /// index and in the material handler, leaving it owned by m_storage but + /// unreachable: consumers wired before the collision kept the first, + /// consumers wired after silently got the second. With mismatched property + /// types that is not a wrong answer but a wrong pointer, because lookups + /// resolve through a static_cast -- a consumer expecting a tensor writes + /// through a pointer to a double. ASan reports a heap-buffer-overflow; + /// without ASan it is silent. + /// + /// Checked here, before construction, rather than in adopt(): a material + /// registers its properties under its own name while constructing, so by the + /// time adopt() sees it the collision has already happened in the property + /// registry. + void check_name_available(const parameter_handler& params) const { + const auto& name = params.template get("name"); + if (m_by_name.contains(name)) + throw std::invalid_argument( + "object_store: a material named '" + name + "' already exists. " + "Material names must be unique -- they are what input wires, " + "material references and property lookups resolve against."); + } + /// Take ownership of a fully constructed material and publish it. /// /// Registration happens HERE and not in material_base's constructor, because @@ -84,9 +118,18 @@ class object_store { m_storage.push_back(std::move(ptr)); } + /// Lets find() look up by string_view without building a std::string. + struct transparent_string_hash { + using is_transparent = void; + std::size_t operator()(std::string_view s) const noexcept { + return std::hash{}(s); + } + }; + std::vector> m_storage; std::vector m_interfaces; - std::unordered_map m_by_name; + std::unordered_map> m_by_name; }; } // namespace numsim::materials diff --git a/include/numsim-materials/core/property_engine.h b/include/numsim-materials/core/property_engine.h index 9418daf..54abc63 100644 --- a/include/numsim-materials/core/property_engine.h +++ b/include/numsim-materials/core/property_engine.h @@ -45,7 +45,14 @@ class property_engine { void update_property(const std::string& material, const std::string& property, const std::unordered_set& exclude = {}) { auto* target = find_in_graph(material, property); - if (!target) return; + // Returning quietly here made a typo indistinguishable from a property that + // legitimately never changes: the driver asks for an update, gets no error, + // and reads a stale value for the rest of the analysis. + if (!target) + throw std::runtime_error( + "update_property(): property '" + material + "::" + property + + "' is not in the graph. Check the material and property names, and " + "that the material publishes this property."); update_property(target, exclude); } diff --git a/include/numsim-materials/materials/tensor_component_stepper.h b/include/numsim-materials/materials/tensor_component_stepper.h index abd9577..aa00091 100755 --- a/include/numsim-materials/materials/tensor_component_stepper.h +++ b/include/numsim-materials/materials/tensor_component_stepper.h @@ -1,6 +1,8 @@ #ifndef TENSOR_COMPONENT_STEPPER_H #define TENSOR_COMPONENT_STEPPER_H +#include +#include #include #include "numsim-materials/core/material_base.h" @@ -35,7 +37,9 @@ class tensor_component_stepper m_tensor(base::template add_output("strain", &tensor_component_stepper::update)), m_inc(base::template get_parameter("increment")), m_indices(base::template get_parameter("indices")) - {} + { + validate_indices(); + } /** * @brief Defines the parameters required by this class. @@ -68,6 +72,30 @@ class tensor_component_stepper } private: + /// "indices" addresses a tensor component directly, so a deck typo used to + /// land in update() as an out-of-bounds subscript: [9,9] on a 3x3 is a read + /// and a write past the end of the tensor (ASan: heap-buffer-overflow), and + /// too few entries read m_indices[1] past the end of the vector itself. + /// Neither is checked by tmech, so this is the only place it can be caught. + /// Checked once here rather than in update(), which runs per increment. + void validate_indices() const { + if (m_indices.size() != Rank) + throw std::invalid_argument( + "tensor_component_stepper ('" + this->name() + "'): \"indices\" has " + + std::to_string(m_indices.size()) + " entr" + + (m_indices.size() == 1 ? "y" : "ies") + ", but a rank-" + + std::to_string(Rank) + " tensor needs exactly " + + std::to_string(Rank) + "."); + + for (std::size_t i = 0; i < m_indices.size(); ++i) + if (m_indices[i] >= Dim) + throw std::invalid_argument( + "tensor_component_stepper ('" + this->name() + "'): \"indices\"[" + + std::to_string(i) + "] is " + std::to_string(m_indices[i]) + + ", which is out of range for a " + std::to_string(Dim) + "D tensor " + "(valid: 0.." + std::to_string(Dim - 1) + ")."); + } + /// Produced properties tensor &m_tensor; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0caebb8..d148f04 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -10,6 +10,7 @@ macro(add_numsim_test TARGET_NAME) endmacro() add_numsim_test(test_property_graph test_property_graph.cpp) +add_numsim_test(test_model_validation test_model_validation.cpp) add_numsim_test(test_materials test_materials.cpp) add_numsim_test(test_damage test_damage.cpp) add_numsim_test(test_property_recorder test_property_recorder.cpp) diff --git a/tests/test_model_validation.cpp b/tests/test_model_validation.cpp new file mode 100644 index 0000000..c9ec28b --- /dev/null +++ b/tests/test_model_validation.cpp @@ -0,0 +1,248 @@ +/// Tests for the checks that reject a malformed model instead of running it. +/// +/// Each of these used to be silent: a duplicate material name aliased one +/// material's properties onto another (and corrupted the heap when their types +/// differed), an out-of-range "indices" subscripted a tensor past its end, an +/// update_property() typo looked like a property that never changes, and +/// commit()/revert() on an unfinalized context did nothing at all. +#include +#include +#include "numsim-materials/core/material_context.h" +#include "numsim-materials/materials/linear_elasticity.h" +#include "numsim-materials/materials/scalar_stepper.h" +#include "numsim-materials/materials/tensor_component_stepper.h" +#include "numsim-materials/default_materials.h" + +namespace { + +using policy = numsim::materials::material_policy_default; +using T = policy::value_type; +using ctx_type = numsim::materials::material_context; +using param_type = policy::ParameterHandler; + +/// Aliased because the comma in the template argument list would otherwise be +/// read as a macro argument separator by EXPECT_THROW. +using stepper2 = numsim::materials::tensor_component_stepper<2, policy>; + +/// A stepper named @p name producing "strain" at component [0,0]. +void add_stepper(ctx_type& ctx, param_type& p, const std::string& name) { + p.clear(); + p.insert("name", name); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0}); + ctx.create(p); +} + +// --- Duplicate material names (#56) --- + +TEST(ModelValidation, ASecondMaterialWithALiveNameIsRejected) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.5}); + p.insert>("indices", {1, 1}); + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); +} + +/// The dangerous case: the two materials publish properties of DIFFERENT +/// types under the same name. Lookups resolve through a static_cast, so +/// before the check this wired a tensor consumer to a double and wrote past +/// the end of it -- a heap-buffer-overflow under ASan, silent without it. +TEST(ModelValidation, ADuplicateNameIsRejectedEvenWhenTheTypesDiffer) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "collide"); + + p.clear(); + p.insert("name", "collide"); + p.insert("increment", T{0.1}); + + EXPECT_THROW(ctx.create>(p), + std::invalid_argument); +} + +TEST(ModelValidation, TheDuplicateNameErrorNamesTheMaterial) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "the_clashing_name"); + + p.clear(); + p.insert("name", "the_clashing_name"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0}); + + try { + ctx.create(p); + FAIL() << "a duplicate material name was accepted"; + } catch (const std::invalid_argument& e) { + EXPECT_NE(std::string(e.what()).find("the_clashing_name"), + std::string::npos) + << "the message does not name the offending material: " << e.what(); + } +} + +/// The rejection must happen before the second material is constructed, or it +/// has already overwritten the first one's entries in the property registry. +/// The surviving material must be the one that was there first. +TEST(ModelValidation, TheFirstMaterialSurvivesARejectedDuplicate) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{99.0}); // would be obvious in the result + p.insert>("indices", {0, 0}); + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); + + p.clear(); + p.insert("name", "elastic"); + p.insert("strain_producer_name", "stepper"); + p.insert("K", T{166.67}); + p.insert("G", T{76.92}); + ctx.create>(p); + + ctx.finalize(); + ctx.update(); + + // One increment of 0.1 from the FIRST stepper, not 99.0 from the rejected one. + const auto& strain = ctx.get>("stepper", "strain"); + EXPECT_DOUBLE_EQ(strain(0, 0), 0.1); +} + +// --- tensor_component_stepper "indices" (#57) --- + +TEST(ModelValidation, AnOutOfRangeIndexIsRejected) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.1}); + p.insert>("indices", {9, 9}); + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); +} + +/// Dim is the last valid index plus one -- the off-by-one a range check is +/// most likely to get wrong. +TEST(ModelValidation, TheIndexRangeCheckIsInclusiveOfTheLastComponent) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "edge"); + p.insert("increment", T{0.1}); + p.insert>("indices", {2, 2}); // valid for 3D + EXPECT_NO_THROW( + ctx.create(p)); + + ctx_type ctx2; + p.clear(); + p.insert("name", "past_edge"); + p.insert("increment", T{0.1}); + p.insert>("indices", {3, 0}); // one past + EXPECT_THROW( + ctx2.create(p), + std::invalid_argument); +} + +/// Too few entries read m_indices[1] past the end of the vector itself, which +/// is a different overflow from the out-of-range one above. +TEST(ModelValidation, AnIndexListOfTheWrongLengthIsRejected) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "short"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0}); // rank 2 needs 2 + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); + + ctx_type ctx2; + p.clear(); + p.insert("name", "long"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0, 0}); + EXPECT_THROW( + ctx2.create(p), + std::invalid_argument); +} + +TEST(ModelValidation, TheIndexErrorReportsTheOffendingValueAndTheRange) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 7}); + + try { + ctx.create(p); + FAIL() << "an out-of-range index was accepted"; + } catch (const std::invalid_argument& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find('7'), std::string::npos) + << "the message does not report the offending value: " << msg; + EXPECT_NE(msg.find("0..2"), std::string::npos) + << "the message does not report the valid range: " << msg; + } +} + +// --- update_property with a name that is not in the graph (#69) --- + +TEST(ModelValidation, UpdatePropertyWithAnUnknownNameThrows) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + ctx.finalize(); + + EXPECT_THROW(ctx.update_property("stepper", "starin"), std::runtime_error); + EXPECT_THROW(ctx.update_property("steppr", "strain"), std::runtime_error); + EXPECT_NO_THROW(ctx.update_property("stepper", "strain")); +} + +TEST(ModelValidation, TheUnknownPropertyErrorNamesBothHalves) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + ctx.finalize(); + + try { + ctx.update_property("stepper", "starin"); + FAIL() << "a property that is not in the graph was accepted"; + } catch (const std::runtime_error& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find("stepper"), std::string::npos) << msg; + EXPECT_NE(msg.find("starin"), std::string::npos) << msg; + } +} + +// --- commit()/revert() before finalize() (#60) --- + +TEST(ModelValidation, CommitAndRevertRequireAFinalizedContext) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + // The engine's history list is built by finalize(), so before it these + // walked an empty list and reported success. + EXPECT_THROW(ctx.commit(), std::logic_error); + EXPECT_THROW(ctx.revert(), std::logic_error); + + ctx.finalize(); + EXPECT_NO_THROW(ctx.commit()); + EXPECT_NO_THROW(ctx.revert()); +} + +} // namespace