Skip to content

[LinearSolver] Fix out-of-bounds write in parallel J M^-1 J^T - #6315

Open
AdoHaha wants to merge 1 commit into
sofa-framework:masterfrom
AdoHaha:fix/parallel-compliance-columnresult
Open

AdoHaha wants to merge 1 commit into
sofa-framework:masterfrom
AdoHaha:fix/parallel-compliance-columnresult

Conversation

@AdoHaha

@AdoHaha AdoHaha commented Sep 17, 2026

Copy link
Copy Markdown

Fixes #6314

Problem

MatrixLinearSolver<Matrix,Vector>::addJMInvJtLocal builds the compliance product fact * J M^-1 J^T with one task per constraint row. Each task fills columnResult[row], which holds column row of the result, but the scratch vector was resized to J->colSize() (the number of mechanical DOFs) while being indexed by row2 (the constraint row).

buildComplianceMatrix sizes J as (result->rowSize(), systemMatrix->colSize()), so J->rowSize() is the number of constraint rows. As soon as there are more constraint rows than DOFs — normal for contact-rich scenes — columnResult[row] is undersized and every column computation writes past its end.

In release builds (NDEBUG) FullVector::operator[] does not bounds-check, so this is silent heap corruption rather than a wrong result; in our simulations it surfaced as malloc(): corrupted top size followed by SIGABRT. The serial path (singleThreadAddJMInvJtLocal) is not affected because it uses the system RHS/solution vectors.

Fix

Size the scratch column with J->rowSize(). FullVector::resize zero-initializes the buffer, so the existing += accumulation remains correct.

How it was tested

A minimal scene with one rigid body (6 DOFs) and N static spheres in contact (FrictionContactConstraint(mu=0.5), i.e. 3N constraint rows), on v26.06.00 with EigenSimplicialLDLT and parallelInverseProduct=true:

case constraint rows DOFs result
2 spheres 6 6 completes
3 spheres 9 6 malloc(): corrupted top size → SIGABRT
10 spheres 30 6 malloc(): corrupted top size → SIGABRT
10 spheres, serial 30 6 completes

Running the same 10-sphere parallel case through the corrected routine (a local subclass changing only this line) does not abort, and its multipliers are exactly equal to the serial run (max abs difference 0.0). Script and logs: https://gist.github.com/AdoHaha/6ba87f1b18e194ad75722b1cce7ab199

I did not add a C++ unit test for this path: addJMInvJtLocal requires a linearSystem link and an initialized task scheduler, so the reproduction above is an integration scene. I am happy to contribute a test if you can point me to the fixture you would prefer for solver/constraint-compliance tests.

AI disclosure

The investigation and this patch were prepared with the help of an AI assistant. The failure was reproduced in a real simulation (contact problem with 940 constraint rows and 246 mechanical coordinates); the contributor reviewed the change. Per CONTRIBUTING.md, this PR should carry the pr: AI-aided label — I could not apply it because this account does not have permission to edit labels in this repository.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

`columnResult[row]` stores column `row` of the compliance matrix, so it must
be sized by `J->rowSize()` (the number of constraint rows) and not by
`J->colSize()` (the number of mechanical DOFs). Whenever the constraint row
count exceeds the DOF count the scratch vector was written past its end,
corrupting the heap in release builds (observed as `malloc(): corrupted top
size`).

Refs sofa-framework#6314

Signed-off-by: Igor <igorzubrycki@gmail.com>
@hugtalbot hugtalbot added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI labels Sep 17, 2026
@hugtalbot

Copy link
Copy Markdown
Contributor

Hi @AdoHaha

Thank you very much for your PR.
We recently updated our policy regarding AI.

"[...] Directly copying AI-generated text into issues, pull request descriptions, or comments is discouraged. Instead, contributors are expected to rephrase or summarize such content in their own words to demonstrate comprehension and ownership of the material [...]

The reason you faced this issue and you came to fix it is also a plus.

I added the flags for you and we will discuss the PR with the core devs.

@AdoHaha

AdoHaha commented Sep 17, 2026

Copy link
Copy Markdown
Author

@hugtalbot ok, next time I will write myself. Note that this was a bug caught during work on actual project: a balloon based device interacting with a plate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parallel addJMInvJtLocal overflows columnResult buffer when constraint rows > DOFs (parallelInverseProduct)

2 participants