From 8ec81f5ed5db8f7941e055726e331c74e84f0f0c Mon Sep 17 00:00:00 2001 From: Rudolf Weeber Date: Mon, 15 Jun 2026 11:21:59 +0200 Subject: [PATCH 1/2] core: bond_breakage: handle ActionType::NONE as a no-op (bug-sweep #33) For an ActionType::NONE pair-bond queue entry, actions_for_breakage fell through to the REVERT/angle-bond else branch and unconditionally evaluated *(e.bond_partners[1]), which is std::nullopt for a pair bond. This is a read of an empty std::optional (undefined behaviour). It traps under _GLIBCXX_ASSERTIONS or a sanitizer, and even in a plain build it dereferences the value-initialized payload 0, resolving to a real non-virtual particle and emitting a spurious REVERT_BIND_AT_POINT_OF_ COLLISION runtime error (or, in other configurations, deleting the wrong bond). NONE entries always queue because check_and_handle_breakage never inspects action_type and breakage_length defaults to 0.0. ActionType::NONE is a documented, valid configuration ("none" in the Python/script-interface layer) and is the value of a default-constructed BreakageSpec, so the correct behaviour is to do nothing. Add an early return of an empty action set for ActionType::NONE at the top of actions_for_breakage. This also resolves finding #34 (NONE on a virtual- site angle bond silently deleting bonds via the same else branch). Add a regression test (none_action_pair_bond_is_noop) that queues a NONE pair-bond entry and checks process_queue is a clean no-op: no exception, no runtime error, and the particles' bonds are preserved. Co-Authored-By: Claude Opus 4.8 --- src/core/bond_breakage/bond_breakage.cpp | 9 +++ src/core/unit_tests/CMakeLists.txt | 3 +- src/core/unit_tests/bond_breakage_test.cpp | 94 ++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/core/bond_breakage/bond_breakage.cpp b/src/core/bond_breakage/bond_breakage.cpp index 296eec5282..2e2b7fae86 100644 --- a/src/core/bond_breakage/bond_breakage.cpp +++ b/src/core/bond_breakage/bond_breakage.cpp @@ -79,6 +79,15 @@ static ActionSet actions_for_breakage(CellStructure const &cell_structure, return bond_partners[1]; }; // optional for second partner engaged + // ActionType::NONE means "do nothing". This is a documented, valid spec + // (and the value of a default-constructed BreakageSpec), so return no + // actions. Without this early return, a NONE pair-bond entry would fall + // through to the REVERT/angle-bond branch below and dereference the empty + // optional bond_partners[1] (undefined behaviour). + if (spec.action_type == ActionType::NONE) { + return {}; + } + // Handle different action types if (spec.action_type == ActionType::DELETE_BOND) { if (is_angle_bond(e.bond_partners)) { diff --git a/src/core/unit_tests/CMakeLists.txt b/src/core/unit_tests/CMakeLists.txt index 59d9122368..cd171e879d 100644 --- a/src/core/unit_tests/CMakeLists.txt +++ b/src/core/unit_tests/CMakeLists.txt @@ -77,7 +77,8 @@ espresso_unit_test(SRC random_test.cpp DEPENDS espresso::utils Random123) espresso_unit_test(SRC BondList_test.cpp DEPENDS espresso::core) espresso_unit_test(SRC energy_test.cpp DEPENDS espresso::core) espresso_unit_test(SRC bonded_interactions_map_test.cpp DEPENDS espresso::core) -espresso_unit_test(SRC bond_breakage_test.cpp DEPENDS espresso::core) +espresso_unit_test(SRC bond_breakage_test.cpp DEPENDS espresso::core Boost::mpi + MPI::MPI_CXX) if(ESPRESSO_BUILD_WITH_WALBERLA) espresso_unit_test(SRC lb_particle_coupling_test.cpp DEPENDS espresso::core Boost::mpi MPI::MPI_CXX NUM_PROC 2) diff --git a/src/core/unit_tests/bond_breakage_test.cpp b/src/core/unit_tests/bond_breakage_test.cpp index cbf23e7f53..4f772ccc9a 100644 --- a/src/core/unit_tests/bond_breakage_test.cpp +++ b/src/core/unit_tests/bond_breakage_test.cpp @@ -21,7 +21,42 @@ #define BOOST_TEST_DYN_LINK #include +#include "EspressoCoreGlobalConfig.hpp" +#include "ParticleFactory.hpp" + #include "bond_breakage/actions.hpp" +#include "bond_breakage/bond_breakage.hpp" + +#include "cell_system/CellStructureType.hpp" +#include "communication.hpp" +#include "errorhandling.hpp" +#include "system/System.hpp" + +#include + +#include +#include + +namespace espresso { +// ESPResSo system instance +static std::shared_ptr system; +} // namespace espresso + +struct GlobalConfig : public EspressoCoreGlobalConfig { + GlobalConfig() { + ErrorHandling::init_error_handling(comm_cart); + espresso::system = System::System::create(); + espresso::system->set_cell_structure_topology(CellStructureType::REGULAR); + ::System::set_system(espresso::system); + } + ~GlobalConfig() { + espresso::system.reset(); + ::System::reset_system(); + ErrorHandling::deinit_error_handling(); + } +}; + +BOOST_TEST_GLOBAL_CONFIGURATION(GlobalConfig); BOOST_AUTO_TEST_CASE(test_actions_equality) { { @@ -76,3 +111,62 @@ BOOST_AUTO_TEST_CASE(test_actions_hash_value) { BOOST_CHECK((Action{1, 2}.hash_value() != Action{1, 0}.hash_value())); } } + +/** + * Regression test for bug-sweep finding #33: an @ref + * BondBreakage::ActionType::NONE breakage spec applied to a pair bond must be a + * clean no-op. On the unfixed code the NONE pair-bond queue entry falls into + * the REVERT/angle-bond @c else branch of @c actions_for_breakage, which + * dereferences @c bond_partners[1] -- an empty @c std::optional for a pair + * bond (the second partner is @c std::nullopt). This is undefined behaviour; + * with libstdc++ assertions (or a sanitizer) it aborts. After the fix the + * NONE entry returns no actions and the particle's bonds are left untouched. + */ +BOOST_FIXTURE_TEST_CASE(none_action_pair_bond_is_noop, ParticleFactory) { + auto &system = *espresso::system; + system.set_box_l(Utils::Vector3d::broadcast(10.)); + system.set_time_step(0.01); + system.cell_structure->set_verlet_skin(0.4); + + auto const comm = boost::mpi::communicator(); + // single-rank scenario: place both partners on the only rank + if (comm.size() != 1) { + return; + } + + auto const pid0 = 0; + auto const pid1 = 1; + auto const bond_type = 0; + create_particle({1.0, 1.0, 1.0}, pid0, 0); + create_particle({1.5, 1.0, 1.0}, pid1, 0); + + // Configure a NONE breakage spec with the default breakage_length of 0.0, + // exactly as a default-constructed core BreakageSpec / Python + // BreakageSpec(action_type="none") would produce. + BondBreakage::BondBreakage bond_breakage{}; + bond_breakage.breakage_specs[bond_type] = + std::make_shared( + BondBreakage::BreakageSpec{0.0, BondBreakage::ActionType::NONE}); + + // Queue a *pair*-bond breakage event: the second partner is std::nullopt. + // This mirrors bond_handler() for a pair bond (partners.size() == 1). + auto const queued = bond_breakage.check_and_handle_breakage( + pid0, {{pid1, std::nullopt}}, bond_type, /* distance */ 0.0); + BOOST_REQUIRE(queued); + + // On the UNFIXED code this dereferences the empty optional bond_partners[1] + // (undefined behaviour -> abort under _GLIBCXX_ASSERTIONS or a sanitizer). + // On the FIXED code it is a clean no-op. + BOOST_CHECK_NO_THROW(bond_breakage.process_queue(system)); + + // Even in a non-hardened build the bug is observable: dereferencing the + // empty optional yields the value-initialized payload 0, which resolves to + // the (non-virtual) particle pid0 and makes the REVERT/angle branch emit a + // spurious "has to be configured for the bond on the virtual site" runtime + // error. A correct NONE no-op must not emit any runtime error. + BOOST_CHECK_EQUAL(check_runtime_errors_local(), 0); + + // The bonds of the involved particles must be left untouched by a NONE spec. + BOOST_CHECK(system.cell_structure->get_local_particle(pid0)->bonds().empty()); + BOOST_CHECK(system.cell_structure->get_local_particle(pid1)->bonds().empty()); +} From b69cc39e6183d588aec5d48288117d9f3a387854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-No=C3=ABl=20Grad?= Date: Wed, 17 Jun 2026 14:45:46 +0200 Subject: [PATCH 2/2] Rewrite proposed patch --- src/core/bond_breakage/bond_breakage.cpp | 101 ++++++++++----------- src/core/unit_tests/CMakeLists.txt | 3 +- src/core/unit_tests/bond_breakage_test.cpp | 94 ------------------- testsuite/python/bond_breakage.py | 44 ++++++++- 4 files changed, 88 insertions(+), 154 deletions(-) diff --git a/src/core/bond_breakage/bond_breakage.cpp b/src/core/bond_breakage/bond_breakage.cpp index 2e2b7fae86..9dcaf7837b 100644 --- a/src/core/bond_breakage/bond_breakage.cpp +++ b/src/core/bond_breakage/bond_breakage.cpp @@ -79,16 +79,6 @@ static ActionSet actions_for_breakage(CellStructure const &cell_structure, return bond_partners[1]; }; // optional for second partner engaged - // ActionType::NONE means "do nothing". This is a documented, valid spec - // (and the value of a default-constructed BreakageSpec), so return no - // actions. Without this early return, a NONE pair-bond entry would fall - // through to the REVERT/angle-bond branch below and dereference the empty - // optional bond_partners[1] (undefined behaviour). - if (spec.action_type == ActionType::NONE) { - return {}; - } - - // Handle different action types if (spec.action_type == ActionType::DELETE_BOND) { if (is_angle_bond(e.bond_partners)) { return {DeleteAngleBond{e.particle_id, @@ -97,54 +87,59 @@ static ActionSet actions_for_breakage(CellStructure const &cell_structure, } return {DeleteBond{e.particle_id, *(e.bond_partners[0]), e.bond_type}}; } + #ifdef ESPRESSO_VIRTUAL_SITES_RELATIVE // revert bind at point of collision for pair bonds - if (spec.action_type == ActionType::REVERT_BIND_AT_POINT_OF_COLLISION and - not is_angle_bond(e.bond_partners)) { - // We need to find the base particles for the two virtual sites - // between which the bond broke. - auto p1 = cell_structure.get_local_particle(e.particle_id); - auto p2 = cell_structure.get_local_particle(*(e.bond_partners[0])); - if (p1 and p2) { - if (not p1->is_virtual() or not p2->is_virtual()) { - runtimeErrorMsg() << "The REVERT_BIND_AT_POINT_OF_COLLISION bond " - "breakage action has to be configured for the " - "bond on the virtual site. Encountered a particle " - "that is not virtual."; - return {}; - } + if (spec.action_type == ActionType::REVERT_BIND_AT_POINT_OF_COLLISION) { + if (not is_angle_bond(e.bond_partners)) { + // We need to find the base particles for the two virtual sites + // between which the bond broke. + auto p1 = cell_structure.get_local_particle(e.particle_id); + auto p2 = cell_structure.get_local_particle(*(e.bond_partners[0])); + if (p1 and p2) { + if (not p1->is_virtual() or not p2->is_virtual()) { + runtimeErrorMsg() + << "The REVERT_BIND_AT_POINT_OF_COLLISION bond " + "breakage action has to be configured for the " + "bond on the virtual site. Encountered a particle " + "that is not virtual."; + return {}; + } - return { - // Bond between virtual sites - DeleteBond{e.particle_id, *(e.bond_partners[0]), e.bond_type}, - // Bond between base particles. We do not know, on which of these - // the bond is defined, since bonds are stored only on one partner - DeleteAllBonds{p1->vs_relative().to_particle_id, - p2->vs_relative().to_particle_id}, - DeleteAllBonds{p2->vs_relative().to_particle_id, - p1->vs_relative().to_particle_id}, - }; - } - } else { - // revert bind at point of collision for angle bonds - auto vs = cell_structure.get_local_particle(e.particle_id); - auto p1 = cell_structure.get_local_particle(*(e.bond_partners[0])); - auto p2 = cell_structure.get_local_particle(*(e.bond_partners[1])); - if (p1 and p2) { - if (not vs->is_virtual()) { - runtimeErrorMsg() << "The REVERT_BIND_AT_POINT_OF_COLLISION bond " - "breakage action has to be configured for the " - "bond on the virtual site. Encountered a particle " - "that is not virtual."; - return {}; + return { + // Bond between virtual sites + DeleteBond{e.particle_id, *(e.bond_partners[0]), e.bond_type}, + // Bond between base particles. We do not know, on which of these + // the bond is defined, since bonds are stored only on one partner + DeleteAllBonds{p1->vs_relative().to_particle_id, + p2->vs_relative().to_particle_id}, + DeleteAllBonds{p2->vs_relative().to_particle_id, + p1->vs_relative().to_particle_id}, + }; } + } else { + // revert bind at point of collision for angle bonds + auto vs = cell_structure.get_local_particle(e.particle_id); + auto p1 = cell_structure.get_local_particle(*(e.bond_partners[0])); + auto p2 = cell_structure.get_local_particle(*(e.bond_partners[1])); + if (p1 and p2) { + if (not vs->is_virtual()) { + runtimeErrorMsg() + << "The REVERT_BIND_AT_POINT_OF_COLLISION bond " + "breakage action has to be configured for the " + "bond on the virtual site. Encountered a particle " + "that is not virtual."; + return {}; + } - return {// Angle bond on the virtual site - DeleteAngleBond{e.particle_id, {p1->id(), p2->id()}, e.bond_type}, - // Bond between base particles. We do not know, on which of these - // the bond is defined, since bonds are stored only on one partner - DeleteAllBonds{p1->id(), p2->id()}, - DeleteAllBonds{p2->id(), p1->id()}}; + return { + // Angle bond on the virtual site + DeleteAngleBond{e.particle_id, {p1->id(), p2->id()}, e.bond_type}, + // Bond between base particles. We do not know, on which of these + // the bond is defined, since bonds are stored only on one partner + DeleteAllBonds{p1->id(), p2->id()}, + DeleteAllBonds{p2->id(), p1->id()}}; + } } } #endif // ESPRESSO_VIRTUAL_SITES_RELATIVE diff --git a/src/core/unit_tests/CMakeLists.txt b/src/core/unit_tests/CMakeLists.txt index cd171e879d..59d9122368 100644 --- a/src/core/unit_tests/CMakeLists.txt +++ b/src/core/unit_tests/CMakeLists.txt @@ -77,8 +77,7 @@ espresso_unit_test(SRC random_test.cpp DEPENDS espresso::utils Random123) espresso_unit_test(SRC BondList_test.cpp DEPENDS espresso::core) espresso_unit_test(SRC energy_test.cpp DEPENDS espresso::core) espresso_unit_test(SRC bonded_interactions_map_test.cpp DEPENDS espresso::core) -espresso_unit_test(SRC bond_breakage_test.cpp DEPENDS espresso::core Boost::mpi - MPI::MPI_CXX) +espresso_unit_test(SRC bond_breakage_test.cpp DEPENDS espresso::core) if(ESPRESSO_BUILD_WITH_WALBERLA) espresso_unit_test(SRC lb_particle_coupling_test.cpp DEPENDS espresso::core Boost::mpi MPI::MPI_CXX NUM_PROC 2) diff --git a/src/core/unit_tests/bond_breakage_test.cpp b/src/core/unit_tests/bond_breakage_test.cpp index 4f772ccc9a..cbf23e7f53 100644 --- a/src/core/unit_tests/bond_breakage_test.cpp +++ b/src/core/unit_tests/bond_breakage_test.cpp @@ -21,42 +21,7 @@ #define BOOST_TEST_DYN_LINK #include -#include "EspressoCoreGlobalConfig.hpp" -#include "ParticleFactory.hpp" - #include "bond_breakage/actions.hpp" -#include "bond_breakage/bond_breakage.hpp" - -#include "cell_system/CellStructureType.hpp" -#include "communication.hpp" -#include "errorhandling.hpp" -#include "system/System.hpp" - -#include - -#include -#include - -namespace espresso { -// ESPResSo system instance -static std::shared_ptr system; -} // namespace espresso - -struct GlobalConfig : public EspressoCoreGlobalConfig { - GlobalConfig() { - ErrorHandling::init_error_handling(comm_cart); - espresso::system = System::System::create(); - espresso::system->set_cell_structure_topology(CellStructureType::REGULAR); - ::System::set_system(espresso::system); - } - ~GlobalConfig() { - espresso::system.reset(); - ::System::reset_system(); - ErrorHandling::deinit_error_handling(); - } -}; - -BOOST_TEST_GLOBAL_CONFIGURATION(GlobalConfig); BOOST_AUTO_TEST_CASE(test_actions_equality) { { @@ -111,62 +76,3 @@ BOOST_AUTO_TEST_CASE(test_actions_hash_value) { BOOST_CHECK((Action{1, 2}.hash_value() != Action{1, 0}.hash_value())); } } - -/** - * Regression test for bug-sweep finding #33: an @ref - * BondBreakage::ActionType::NONE breakage spec applied to a pair bond must be a - * clean no-op. On the unfixed code the NONE pair-bond queue entry falls into - * the REVERT/angle-bond @c else branch of @c actions_for_breakage, which - * dereferences @c bond_partners[1] -- an empty @c std::optional for a pair - * bond (the second partner is @c std::nullopt). This is undefined behaviour; - * with libstdc++ assertions (or a sanitizer) it aborts. After the fix the - * NONE entry returns no actions and the particle's bonds are left untouched. - */ -BOOST_FIXTURE_TEST_CASE(none_action_pair_bond_is_noop, ParticleFactory) { - auto &system = *espresso::system; - system.set_box_l(Utils::Vector3d::broadcast(10.)); - system.set_time_step(0.01); - system.cell_structure->set_verlet_skin(0.4); - - auto const comm = boost::mpi::communicator(); - // single-rank scenario: place both partners on the only rank - if (comm.size() != 1) { - return; - } - - auto const pid0 = 0; - auto const pid1 = 1; - auto const bond_type = 0; - create_particle({1.0, 1.0, 1.0}, pid0, 0); - create_particle({1.5, 1.0, 1.0}, pid1, 0); - - // Configure a NONE breakage spec with the default breakage_length of 0.0, - // exactly as a default-constructed core BreakageSpec / Python - // BreakageSpec(action_type="none") would produce. - BondBreakage::BondBreakage bond_breakage{}; - bond_breakage.breakage_specs[bond_type] = - std::make_shared( - BondBreakage::BreakageSpec{0.0, BondBreakage::ActionType::NONE}); - - // Queue a *pair*-bond breakage event: the second partner is std::nullopt. - // This mirrors bond_handler() for a pair bond (partners.size() == 1). - auto const queued = bond_breakage.check_and_handle_breakage( - pid0, {{pid1, std::nullopt}}, bond_type, /* distance */ 0.0); - BOOST_REQUIRE(queued); - - // On the UNFIXED code this dereferences the empty optional bond_partners[1] - // (undefined behaviour -> abort under _GLIBCXX_ASSERTIONS or a sanitizer). - // On the FIXED code it is a clean no-op. - BOOST_CHECK_NO_THROW(bond_breakage.process_queue(system)); - - // Even in a non-hardened build the bug is observable: dereferencing the - // empty optional yields the value-initialized payload 0, which resolves to - // the (non-virtual) particle pid0 and makes the REVERT/angle branch emit a - // spurious "has to be configured for the bond on the virtual site" runtime - // error. A correct NONE no-op must not emit any runtime error. - BOOST_CHECK_EQUAL(check_runtime_errors_local(), 0); - - // The bonds of the involved particles must be left untouched by a NONE spec. - BOOST_CHECK(system.cell_structure->get_local_particle(pid0)->bonds().empty()); - BOOST_CHECK(system.cell_structure->get_local_particle(pid1)->bonds().empty()); -} diff --git a/testsuite/python/bond_breakage.py b/testsuite/python/bond_breakage.py index a071431331..e0e57491de 100644 --- a/testsuite/python/bond_breakage.py +++ b/testsuite/python/bond_breakage.py @@ -253,18 +253,15 @@ def count_bonds(self, pairs): return bonds_count def setUp(self): - - box_vol = self.system.box_l[0]**3. phi = 0.4 - r = 1. - solid_vol = phi * box_vol + solid_vol = phi * self.system.volume() part_vol = 4 / 3 * np.pi * r**3 part_num = int(solid_vol / part_vol) np.random.seed(seed=678) for i in range(part_num): - pos = np.random.rand(3) * self.system.box_l[0] + pos = np.random.rand(3) * self.system.box_l self.system.part.add(pos=pos) self.system.non_bonded_inter[0, 0].lennard_jones.set_params( @@ -360,5 +357,42 @@ def test_vs_bonds(self): np.testing.assert_equal(bonds_dist, bonds_count) +class BreakageAPI(BondBreakageCommon, ut.TestCase): + + def setUp(self): + self.system.box_l = 3 * [20] + self.system.min_global_cut = 0.1 + self.system.time_step = 0.01 + self.system.cell_system.skin = 0.4 + + def tearDown(self): + self.system.part.clear() + self.system.bonded_inter.clear() + + def test_bond_deletion(self): + p1 = self.system.part.add(pos=[0., 0., 0.], v=[0., 0., 0.]) + p2 = self.system.part.add(pos=[0., 0., 1.], v=[0., 0., 1.]) + bond = espressomd.interactions.HarmonicBond(k=1., r_0=1., r_cut=1.2) + self.system.bonded_inter.add(bond) + p1.bonds = ((bond, p2)) + self.system.bond_breakage[bond] = BreakageSpec( + breakage_length=1.1, action_type="delete_bond") + self.system.integrator.run(100) + assert np.linalg.norm(p2.pos - p1.pos) > bond.r_cut + self.assertEqual(len(p1.bonds), 0) + + def test_none(self): + p1 = self.system.part.add(pos=[0., 0., 0.], v=[0., 0., 0.]) + p2 = self.system.part.add(pos=[0., 0., 1.], v=[0., 0., 1.]) + bond = espressomd.interactions.HarmonicBond(k=1., r_0=1., r_cut=1.2) + self.system.bonded_inter.add(bond) + p1.bonds = ((bond, p2)) + self.system.bond_breakage[bond] = BreakageSpec( + breakage_length=1.1, action_type="none") + self.system.integrator.run(100) + assert np.linalg.norm(p2.pos - p1.pos) > bond.r_cut + self.assertEqual(len(p1.bonds), 1) + + if __name__ == "__main__": ut.main()