From 6f2f8448e4d0aec5781958e44e27b67fcb356ff0 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Thu, 4 Dec 2025 10:51:02 +0000 Subject: [PATCH] Correct null pointer dereference in setting a null outcome This fixes a regression introduced when moving to using `std::shared_ptr` to handle reference-counting for game objects. Fixes #625 and #647. --- ChangeLog | 2 ++ src/games/game.h | 5 ++++- src/games/gameobject.h | 20 ++++++++------------ src/games/gametree.cc | 8 ++++---- src/games/gametree.h | 2 +- tests/test_node.py | 9 ++++++++- 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5af871f2d..e319a0744 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,8 @@ extensive form. - Fixed a regression in the GUI in which unique action labels were not being generated when adding a move via drag-and-drop of a player icon (#618) +- Fixed a regression generating null pointer dereference errors when setting the outcome of + a node to the null outcome (#625, #647) ## [16.3.2] - unreleased diff --git a/src/games/game.h b/src/games/game.h index 112b68d38..055fdceb7 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -775,7 +775,10 @@ class GameRep : public std::enable_shared_from_this { throw UndefinedException(); } virtual void DeleteAction(GameAction) { throw UndefinedException(); } - virtual void SetOutcome(GameNode, const GameOutcome &p_outcome) { throw UndefinedException(); } + virtual void SetOutcome(const GameNode &p_node, const GameOutcome &p_outcome) + { + throw UndefinedException(); + } /// @name Dimensions of the game //@{ diff --git a/src/games/gameobject.h b/src/games/gameobject.h index 2246caf02..9e9338cb7 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -96,14 +96,10 @@ template class GameObjectPtr { /// game element held by this object and throws an exception if the object is the /// null object, or is no longer valid (has been removed from the game) /// - /// @exception NullException if the object holds a reference to a null element /// @exception InvalidObjectException if the element referred to has been deleted from its game std::shared_ptr get_shared() const { - if (!m_rep) { - throw NullException(); - } - if (!m_rep->IsValid()) { + if (m_rep && !m_rep->IsValid()) { throw InvalidObjectException(); } return m_rep; @@ -129,16 +125,16 @@ template class GameObjectPtr { } bool operator==(const GameObjectPtr &r) const { return (m_rep == r.m_rep); } - bool operator==(const std::shared_ptr r) const { return (m_rep == r); } - bool operator==(const std::shared_ptr r) const { return (m_rep == r); } - bool operator==(const std::nullptr_t) const { return !bool(m_rep); } + bool operator==(const std::shared_ptr &r) const { return (m_rep == r); } + bool operator==(const std::shared_ptr &r) const { return (m_rep == r); } + bool operator==(const std::nullptr_t &) const { return m_rep == nullptr; } bool operator!=(const GameObjectPtr &r) const { return (m_rep != r.m_rep); } - bool operator!=(const std::shared_ptr r) const { return (m_rep != r); } - bool operator!=(const std::shared_ptr r) const { return (m_rep != r); } - bool operator!=(const std::nullptr_t) const { return bool(m_rep); } + bool operator!=(const std::shared_ptr &r) const { return (m_rep != r); } + bool operator!=(const std::shared_ptr &r) const { return (m_rep != r); } + bool operator!=(const std::nullptr_t &) const { return m_rep != nullptr; } bool operator<(const GameObjectPtr &r) const { return (m_rep < r.m_rep); } - operator bool() const noexcept { return bool(m_rep); } + operator bool() const noexcept { return m_rep != nullptr; } operator std::shared_ptr() const { return m_rep; } }; diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 4fbebf5c5..518dc79cb 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -334,17 +334,17 @@ void GameNodeRep::DeleteOutcome(GameOutcomeRep *outc) } } -void GameTreeRep::SetOutcome(GameNode p_node, const GameOutcome &p_outcome) +void GameTreeRep::SetOutcome(const GameNode &p_node, const GameOutcome &p_outcome) { - IncrementVersion(); if (p_node->m_game != this) { throw MismatchException(); } if (p_outcome && p_outcome->m_game != this) { throw MismatchException(); } - if (p_outcome.get() != p_node->m_outcome) { - p_node->m_outcome = p_outcome.get(); + if (const auto newOutcome = p_outcome.get_shared().get(); newOutcome != p_node->m_outcome) { + p_node->m_outcome = newOutcome; + IncrementVersion(); ClearComputedValues(); } } diff --git a/src/games/gametree.h b/src/games/gametree.h index 8695bb3f3..703ce593e 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -137,7 +137,7 @@ class GameTreeRep : public GameExplicitRep { Game SetChanceProbs(const GameInfoset &, const Array &) override; GameAction InsertAction(GameInfoset, GameAction p_where = nullptr) override; void DeleteAction(GameAction) override; - void SetOutcome(GameNode, const GameOutcome &p_outcome) override; + void SetOutcome(const GameNode &p_node, const GameOutcome &p_outcome) override; std::vector GetPlays(GameNode node) const override; std::vector GetPlays(GameInfoset infoset) const override; diff --git a/tests/test_node.py b/tests/test_node.py index 4104e0e64..48c2740a9 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -19,10 +19,17 @@ def test_get_infoset(): def test_get_outcome(): """Test to ensure that we can retrieve an outcome for a given node""" game = games.read_from_file("basic_extensive_game.efg") - assert game.root.children[0].children[1].children[0].outcome == game.outcomes[1] + assert game.root.children[0].children[1].children[0].outcome == game.outcomes["Outcome 1"] assert game.root.outcome is None +def test_set_outcome_null(): + """Test to set an outcome to the null outcome.""" + game = games.read_from_file("basic_extensive_game.efg") + game.set_outcome(game.root.children[0].children[0].children[0], None) + assert game.root.children[0].children[0].children[0].outcome is None + + def test_get_player(): """Test to ensure that we can retrieve a player for a given node""" game = games.read_from_file("basic_extensive_game.efg")