Conversation
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.
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
1 participant
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.
Four checks a model could previously fail silently, from the repo-wide review. Closes #56, #57, #60, #69; also #64.
What was wrong
A duplicate material name aliased one material onto another (#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. Consumers wired before the collision kept the first, consumers wired after got the second. With mismatched property types it is a wrong pointer, not a wrong answer: lookups resolve through a
static_cast, so a tensor consumer writes through a pointer to a double. ASan reports a heap-buffer-overflow; without ASan it is silent.Checked before construction, not in
adopt()-- a material registers its properties while constructing, so by the timeadopt()runs the collision has already happened in the property registry.tensor_component_stepper's"indices"went to the subscript unchecked (#57).[9,9]on a 3x3 read and wrote past the end of the tensor; a one-element list read past the end of the vector itself. tmech checks neither, so the constructor is the only place it can be caught. Validated once there rather than inupdate(), which runs per increment.update_property()returned quietly for a name not in the graph (#69), 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()skippedcheck_finalized(#60). The engine's history list is built byfinalize(), so before it they walked an empty list and reported success.object_store::find()wasnoexceptbut built astd::string(#64). Abad_allocundernoexceptisstd::terminate. A transparent hash lets it hash thestring_viewdirectly, which also removes the allocation from every lookup.Tests
tests/test_model_validation.cpp, 11 tests. 311/311 pass.Every check was mutation tested -- reverted one at a time, rebuilt, full suite run:
if (false)if (false)if (false)AnIndexListOfTheWrongLengthIsRejectedupdate_propertythrow ->returncommit()'scheck_finalized-> no-opCommitAndRevertRequireAFinalizedContextNo mutant survived, and each was killed only by its own tests.
Two tests exist because they would otherwise be vacuous:
TheIndexRangeCheckIsInclusiveOfTheLastComponentpins the>= Dimboundary (index 2 valid, 3 not) since an off-by-one here is the likely error, andTheFirstMaterialSurvivesARejectedDuplicatechecks the surviving material is the one registered first -- which is what makes rejecting before construction load-bearing rather than cosmetic.