Fix heap-buffer-overflow in ResidualBasedEliminationBuilderAndSolverWithConstraints#14562
Open
FrogZR wants to merge 1 commit into
Open
Conversation
…ithConstraints AssembleRHSWithoutConstraints looked up a DOF via "BaseType::mDofSet.begin() + i_global", treating the equation ID as if it were a container position in mDofSet. mDofSet is sorted by DOF identity (see SetUpDofSetWithConstraints), while equation IDs are assigned via two independent counters (ascending for free DOFs, descending for master-fixed ones) in that same identity-sorted iteration order, so a DOF's equation ID has no fixed relationship to its position in mDofSet. This fetched an arbitrary, usually wrong Dof, corrupting the is_master_fixed/is_slave classification and the mReactionEquationIdMap lookup, causing a heap-buffer-overflow (out-of-bounds read/write into the reactions vector) whenever MasterSlaveConstraints, genuinely fixed DOFs, and reactions were all in use at once. Fixed by classifying purely via equation_id, using a new mReactionCategoryByEquationId map built alongside mReactionEquationIdMap in SetUpSystemWithConstraints (which already iterates real Dof objects correctly). Confirmed with AddressSanitizer: the heap-buffer-overflow no longer occurs, and a model that previously crashed 100% of the time (5+ consecutive repro attempts under both a Debug+ASan build and a plain optimized Release build) now runs multiple time steps to completion. Fixes KratosMultiphysics#14561
Member
Thanks for the honesty |
loumalouomega
left a comment
loumalouomega
left a comment
Member
There was a problem hiding this comment.
LGTM, I think this fixes the issue found by @AlejandroCornejo
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
2 participants
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.
Summary
Fixes #14561.
AssembleRHSWithoutConstraintslooked up a DOF viaBaseType::mDofSet.begin() + i_global, treating the equation ID as if it were acontainer position in
mDofSet.mDofSetis sorted by DOF identity (seeSetUpDofSetWithConstraints), while equation IDs are assigned via two independentcounters (ascending for free DOFs, descending for master-fixed ones) in that same
identity-sorted iteration order — so a DOF's equation ID has no fixed relationship to
its position in
mDofSet.This fetched an arbitrary, usually wrong
Dof, corrupting theis_master_fixed/is_slaveclassification and themReactionEquationIdMaplookup, causing aheap-buffer-overflow (out-of-bounds read/write into the reactions vector) whenever
MasterSlaveConstraints, genuinely fixed DOFs, and reactions were all in use at once.Fix
Classify purely via
equation_id, using a newmReactionCategoryByEquationIdmapbuilt alongside the existing
mReactionEquationIdMapinSetUpSystemWithConstraints(which already iterates real
Dofobjects correctly, unlike the buggy lookup).Test plan
kratos/utilities/atomic_utilities.h:60(viaAtomicAdd, called fromAssembleRHSWithoutConstraints) no longer occurs.with
double free or corruption (!prev)/SIGABRT100% of the time (5+ consecutiverepro attempts, always at the identical point) now runs multiple time steps to
Analysis -END-without any crash.🤖 Generated with Claude Code