Correct null pointer dereference in setting a null outcome (#666)

This fixes a regression introduced when moving to using `std::shared_ptr`
to handle reference-counting for game objects.

Fixes #625 and #647.

Author: tturocy

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

src/games/game.h

@@ -775,7 +775,10 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
     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
   //@{

src/games/gameobject.h

@@ -96,14 +96,10 @@ template <class T> 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<T> 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 T> class GameObjectPtr {
   }
 
   bool operator==(const GameObjectPtr<T> &r) const { return (m_rep == r.m_rep); }
-  bool operator==(const std::shared_ptr<T> r) const { return (m_rep == r); }
-  bool operator==(const std::shared_ptr<const T> r) const { return (m_rep == r); }
-  bool operator==(const std::nullptr_t) const { return !bool(m_rep); }
+  bool operator==(const std::shared_ptr<T> &r) const { return (m_rep == r); }
+  bool operator==(const std::shared_ptr<const T> &r) const { return (m_rep == r); }
+  bool operator==(const std::nullptr_t &) const { return m_rep == nullptr; }
   bool operator!=(const GameObjectPtr<T> &r) const { return (m_rep != r.m_rep); }
-  bool operator!=(const std::shared_ptr<T> r) const { return (m_rep != r); }
-  bool operator!=(const std::shared_ptr<const T> r) const { return (m_rep != r); }
-  bool operator!=(const std::nullptr_t) const { return bool(m_rep); }
+  bool operator!=(const std::shared_ptr<T> &r) const { return (m_rep != r); }
+  bool operator!=(const std::shared_ptr<const T> &r) const { return (m_rep != r); }
+  bool operator!=(const std::nullptr_t &) const { return m_rep != nullptr; }
   bool operator<(const GameObjectPtr<T> &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<T>() const { return m_rep; }
 };
 

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();
   }
 }

src/games/gametree.h

@@ -137,7 +137,7 @@ class GameTreeRep : public GameExplicitRep {
   Game SetChanceProbs(const GameInfoset &, const Array<Number> &) 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<GameNode> GetPlays(GameNode node) const override;
   std::vector<GameNode> GetPlays(GameInfoset infoset) const override;

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")