From efddce85910503a52fb303bea97a172d6c1bfbc4 Mon Sep 17 00:00:00 2001 From: SYMSCAE Date: Tue, 3 Nov 2026 10:13:20 -0800 Subject: [PATCH 1/2] Fix lifetime management regressions in Atom bindings --- news/fix-atom-scatteringpower-lifetime.rst | 24 ++++++++++++++++++++++ src/extensions/nb_atom.cpp | 8 +++++--- 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 news/fix-atom-scatteringpower-lifetime.rst diff --git a/news/fix-atom-scatteringpower-lifetime.rst b/news/fix-atom-scatteringpower-lifetime.rst new file mode 100644 index 0000000..90042c5 --- /dev/null +++ b/news/fix-atom-scatteringpower-lifetime.rst @@ -0,0 +1,24 @@ +**Added:** + +* + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* Preserve scattering power lifetimes in nanobind Atom construction, + copying, and initialization to prevent use-after-free crashes. + +**Security:** + +* diff --git a/src/extensions/nb_atom.cpp b/src/extensions/nb_atom.cpp index 8516522..6450029 100644 --- a/src/extensions/nb_atom.cpp +++ b/src/extensions/nb_atom.cpp @@ -56,7 +56,7 @@ void wrap_atom(nb::module_& m) .def("__init__", [](Atom* self, const Atom& old) { new (self) Atom(old); - }, nb::arg("old")) + }, nb::arg("old"), nb::keep_alive<1,2>()) .def("__init__", [](Atom* self, REAL x, REAL y, REAL z, const std::string& name, nb::object pow, REAL popu) { @@ -64,7 +64,8 @@ void wrap_atom(nb::module_& m) new (self) Atom(x, y, z, name, sp, popu); }, nb::arg("x"), nb::arg("y"), nb::arg("z"), nb::arg("name"), - nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0) + nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0, + nb::keep_alive<1,6>()) .def("Init", [](Atom& self, REAL x, REAL y, REAL z, const std::string& name, nb::object pow, REAL popu) { @@ -72,7 +73,8 @@ void wrap_atom(nb::module_& m) self.Init(x, y, z, name, sp, popu); }, nb::arg("x"), nb::arg("y"), nb::arg("z"), nb::arg("name"), - nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0) + nb::arg("pow").none(), nb::arg("popu") = (REAL)1.0, + nb::keep_alive<1,6>()) .def("GetMass", &Atom::GetMass) .def("GetRadius", &Atom::GetRadius) .def("IsDummy", &Atom::IsDummy) From 12b9087c5bd5baf7a872bcbecdf8b24af8e8b638 Mon Sep 17 00:00:00 2001 From: Vincent Favre-Nicolin Date: Wed, 4 Nov 2026 08:42:20 +0100 Subject: [PATCH 2/2] test: add Atom/ScatteringPower lifetime tests (#123) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_atom.py | 118 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 tests/test_atom.py diff --git a/tests/test_atom.py b/tests/test_atom.py new file mode 100644 index 0000000..38f0e18 --- /dev/null +++ b/tests/test_atom.py @@ -0,0 +1,118 @@ +#!/usr/bin/env python +"""Lifetime tests for Atom and its ScatteringPower (see issue #123). + +An Atom only stores a raw pointer to its ScatteringPower and calls +``DeRegisterClient`` on it in its destructor, so the Python Atom must keep +the Python ScatteringPower alive. +""" + +import gc +import os +import subprocess +import sys +import textwrap + +import pytest + +from pyobjcryst.atom import Atom +from pyobjcryst.scatteringpower import ScatteringPowerAtom + + +def _refcount(obj): + gc.collect() + return sys.getrefcount(obj) + + +# The refcount tests below never let a ScatteringPower die before its Atom, +# so that they fail cleanly (instead of crashing pytest) without the fix. + + +def test_constructor_keeps_scattering_power_alive(): + sp = ScatteringPowerAtom("Ni", "Ni") + before = _refcount(sp) + atom = Atom(0, 0, 0, "Ni", sp) + delta = _refcount(sp) - before + del atom + assert delta >= 1 + + +def test_init_keeps_scattering_power_alive(): + sp1 = ScatteringPowerAtom("Ni", "Ni") + sp2 = ScatteringPowerAtom("O", "O") + atom = Atom(0, 0, 0, "Ni", sp1) + before = _refcount(sp2) + atom.Init(0.1, 0.2, 0.3, "O", sp2) + delta = _refcount(sp2) - before + symbol = atom.GetScatteringPower().GetSymbol() + del atom + assert delta >= 1 + assert symbol == "O" + + +def test_copy_constructor_keeps_original_alive(): + sp = ScatteringPowerAtom("Ni", "Ni") + atom = Atom(0, 0, 0, "Ni", sp) + before = _refcount(atom) + copy = Atom(atom) + delta = _refcount(atom) - before + symbol = copy.GetScatteringPower().GetSymbol() + del copy, atom + assert delta >= 1 + assert symbol == "Ni" + + +@pytest.mark.parametrize( + "script", + [ + # Atom constructed from a temporary ScatteringPower. + """ + atom = Atom(0, 0, 0, "Ni", ScatteringPowerAtom("Ni", "Ni")) + gc.collect() + assert atom.GetScatteringPower().GetSymbol() == "Ni" + del atom + """, + # Atom re-initialized with a temporary ScatteringPower. + """ + atom = Atom(0, 0, 0, "Ni", ScatteringPowerAtom("Ni", "Ni")) + atom.Init(0, 0, 0, "O", ScatteringPowerAtom("O", "O")) + gc.collect() + assert atom.GetScatteringPower().GetSymbol() == "O" + del atom + """, + # Copy outliving both the original Atom and the ScatteringPower. + """ + sp = ScatteringPowerAtom("Ni", "Ni") + atom = Atom(0, 0, 0, "Ni", sp) + copy = Atom(atom) + del sp, atom + gc.collect() + assert copy.GetScatteringPower().GetSymbol() == "Ni" + del copy + """, + # Dummy atom (no ScatteringPower) must still work. + """ + atom = Atom(0, 0, 0, "dummy", None) + assert atom.IsDummy() + assert atom.GetScatteringPower() is None + atom.Init(0, 0, 0, "dummy2", None) + assert atom.IsDummy() + del atom + """, + ], +) +def test_scattering_power_outlived_by_atom(script): + # Run in a subprocess so that a use-after-free crash is reported as a + # test failure rather than killing pytest. Freed memory is scribbled + # (macOS / glibc) to make such a crash deterministic. + code = ( + "import gc\n" + "from pyobjcryst.atom import Atom\n" + "from pyobjcryst.scatteringpower import ScatteringPowerAtom\n" + + textwrap.dedent(script) + + "gc.collect()\n" + ) + env = dict(os.environ, MallocScribble="1", MALLOC_PERTURB_="85") + result = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True, env=env + ) + assert result.returncode == 0, result.stderr