Skip to content

Fix use-after-free of meshes shared across finalized models#3637

Open
caugonnet wants to merge 1 commit into
newton-physics:mainfrom
caugonnet:caugonnet/fix-mesh-finalize-keepalive
Open

Fix use-after-free of meshes shared across finalized models#3637
caugonnet wants to merge 1 commit into
newton-physics:mainfrom
caugonnet:caugonnet/fix-mesh-finalize-keepalive

Conversation

@caugonnet

@caugonnet caugonnet commented Jul 24, 2026

Copy link
Copy Markdown

ModelBuilder.finalize() stores only the raw uint64 wp.Mesh id in model.shape_source_ptr, while Mesh.finalize() rebound self.mesh to a new wp.Mesh on every call. Since replicate() and add_builder() share geometry objects by reference, finalizing a second model built from the same source released the wp.Mesh referenced by the first model, leaving its shape_source_ptr dangling; mesh queries then walked freed BVH memory (CUDA illegal memory access).

Fix it on two levels so neither path can dangle:

  • Mesh.finalize() now caches the finalized wp.Mesh per device (and requires_grad/bvh_constructor) instead of clobbering the previous one, so re-finalizing shared geometry returns the same mesh id. The cache is invalidated when vertices or indices are reassigned.
  • ModelBuilder.finalize() keeps model-owned strong references to every finalized wp.Mesh (Model._mesh_keep_alive), mirroring the existing heightfield_meshes keep-alive, so a model's meshes stay alive for the model's lifetime independent of the geometry objects.

Description

Checklist

  • New or existing tests cover these changes
  • The documentation is up to date with these changes
  • CHANGELOG.md has been updated (if user-facing change)

Test plan

Bug fix

Steps to reproduce:

Minimal reproduction:

import newton

# Code that demonstrates the bug

New feature / API change

import newton

# Code that demonstrates the new capability

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where finalizing multiple models with shared mesh geometry could invalidate meshes used by earlier models.
    • Improved mesh reuse across models and devices while ensuring updated mesh data is correctly reflected.
    • Preserved mesh availability for the full lifetime of finalized models, including replicated geometry.
  • Tests

    • Added regression coverage for shared mesh lifetime, reuse, garbage collection, and mesh queries.

ModelBuilder.finalize() stores only the raw uint64 wp.Mesh id in
model.shape_source_ptr, while Mesh.finalize() rebound self.mesh to a
new wp.Mesh on every call. Since replicate() and add_builder() share
geometry objects by reference, finalizing a second model built from
the same source released the wp.Mesh referenced by the first model,
leaving its shape_source_ptr dangling; mesh queries then walked freed
BVH memory (CUDA illegal memory access).

Fix it on two levels so neither path can dangle:

- Mesh.finalize() now caches the finalized wp.Mesh per device (and
  requires_grad/bvh_constructor) instead of clobbering the previous
  one, so re-finalizing shared geometry returns the same mesh id. The
  cache is invalidated when vertices or indices are reassigned.
- ModelBuilder.finalize() keeps model-owned strong references to every
  finalized wp.Mesh (Model._mesh_keep_alive), mirroring the existing
  heightfield_meshes keep-alive, so a model's meshes stay alive for
  the model's lifetime independent of the geometry objects.

Signed-off-by: Cedric AUGONNET <caugonnet@nvidia.com>
@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@caugonnet
caugonnet requested a deployment to external-pr-approval July 24, 2026 07:59 — with GitHub Actions Waiting
@caugonnet
caugonnet requested a deployment to external-pr-approval July 24, 2026 07:59 — with GitHub Actions Waiting
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: f2da1f03-134b-4969-9ec4-c57599d7c483

📥 Commits

Reviewing files that changed from the base of the PR and between d9139fe and 1f0ff03.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • newton/_src/geometry/types.py
  • newton/_src/sim/builder.py
  • newton/_src/sim/model.py
  • newton/tests/test_builder_replicate.py

📝 Walkthrough

Walkthrough

Mesh finalization now caches device-specific Warp meshes and invalidates caches when geometry changes. Models retain finalized mesh references, and a replication regression test verifies shared meshes survive subsequent finalization and garbage collection.

Changes

Shared mesh lifetime protection

Layer / File(s) Summary
Device-aware mesh finalization cache
newton/_src/geometry/types.py
Mesh.finalize() caches finalized Warp meshes by device and build parameters, while vertex and index reassignment clears the cache.
Model-owned mesh lifetime
newton/_src/sim/model.py, newton/_src/sim/builder.py
Model construction stores finalized mesh references in _mesh_keep_alive during collision geometry finalization.
Shared mesh lifetime regression coverage
newton/tests/test_builder_replicate.py, CHANGELOG.md
A device-wide replication test validates mesh survival, shared mesh IDs, and AABB queries after garbage collection; the changelog records the fixed use-after-free.
Estimated code review effort: 3 (Moderate) ~20 minutes

Possibly related PRs

  • newton-physics/newton#2864: Adds BvhConfig forwarding of bvh_constructor into Mesh.finalize, overlapping with the new finalized-mesh cache key.

Suggested reviewers: adenzler-nvidia, stafah, daniela-hase

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main fix for shared meshes being freed after model finalization.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant