From 5c9705c21c551c072dc0861965d6e8f4b7cbe657 Mon Sep 17 00:00:00 2001
From: rdb <git@rdb.name>
Date: Thu, 24 May 2018 22:43:48 +0200
Subject: [PATCH] pgraph: fix crash accessing python_tags via dict property

Fixes #326
---
 panda/src/pgraph/pandaNode_ext.cxx | 30 ++++++++++++++++++++----------
 panda/src/pgraph/pandaNode_ext.h   |  2 ++
 tests/pgraph/test_nodepath.py      | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/panda/src/pgraph/pandaNode_ext.cxx b/panda/src/pgraph/pandaNode_ext.cxx
index aa24e623ee..4f0a1b4e8d 100644
--- a/panda/src/pgraph/pandaNode_ext.cxx
+++ b/panda/src/pgraph/pandaNode_ext.cxx
@@ -94,14 +94,9 @@ get_tag_keys() const {
  */
 PyObject *Extension<PandaNode>::
 get_python_tags() {
-  if (_this->_python_tag_data == NULL) {
-    _this->_python_tag_data = new PythonTagDataImpl;
-
-  } else if (_this->_python_tag_data->get_ref_count() > 1) {
-    // Copy-on-write.
-    _this->_python_tag_data = new PythonTagDataImpl(*(PythonTagDataImpl *)_this->_python_tag_data.p());
-  }
-  return ((PythonTagDataImpl *)_this->_python_tag_data.p())->_dict;
+  PyObject *dict = do_get_python_tags();
+  Py_INCREF(dict);
+  return dict;
 }
 
 /**
@@ -116,7 +111,7 @@ get_python_tags() {
  */
 int Extension<PandaNode>::
 set_python_tag(PyObject *key, PyObject *value) {
-  return PyDict_SetItem(get_python_tags(), key, value);
+  return PyDict_SetItem(do_get_python_tags(), key, value);
 }
 
 /**
@@ -166,7 +161,7 @@ clear_python_tag(PyObject *key) {
     return;
   }
 
-  PyObject *dict = get_python_tags();
+  PyObject *dict = do_get_python_tags();
   if (PyDict_GetItem(dict, key) != NULL) {
     PyDict_DelItem(dict, key);
   }
@@ -201,6 +196,21 @@ __traverse__(visitproc visit, void *arg) {
   return 0;
 }
 
+/**
+ * Same as get_python_tags, without incrementing the reference count.
+ */
+PyObject *Extension<PandaNode>::
+do_get_python_tags() {
+  if (_this->_python_tag_data == NULL) {
+    _this->_python_tag_data = new PythonTagDataImpl;
+
+  } else if (_this->_python_tag_data->get_ref_count() > 1) {
+    // Copy-on-write.
+    _this->_python_tag_data = new PythonTagDataImpl(*(PythonTagDataImpl *)_this->_python_tag_data.p());
+  }
+  return ((PythonTagDataImpl *)_this->_python_tag_data.p())->_dict;
+}
+
 /**
  * Destroys the tags associated with the node.
  */
diff --git a/panda/src/pgraph/pandaNode_ext.h b/panda/src/pgraph/pandaNode_ext.h
index 86527d42d4..d19bc5475f 100644
--- a/panda/src/pgraph/pandaNode_ext.h
+++ b/panda/src/pgraph/pandaNode_ext.h
@@ -45,6 +45,8 @@ public:
   int __traverse__(visitproc visit, void *arg);
 
 private:
+  PyObject *do_get_python_tags();
+
   // This is what actually stores the Python tags.
   class PythonTagDataImpl : public PandaNode::PythonTagData {
   public:
diff --git a/tests/pgraph/test_nodepath.py b/tests/pgraph/test_nodepath.py
index d60aa312ba..1b98da32d7 100644
--- a/tests/pgraph/test_nodepath.py
+++ b/tests/pgraph/test_nodepath.py
@@ -1,3 +1,5 @@
+import pytest, sys
+
 def test_nodepath_empty():
     """Tests NodePath behavior for empty NodePaths."""
     from panda3d.core import NodePath
@@ -103,3 +105,21 @@ def test_weak_nodepath_comparison():
     assert hash(path) == hash(weak)
     assert weak.get_node_path() == path
     assert weak.node() == path.node()
+
+
+def test_nodepath_python_tags():
+    from panda3d.core import NodePath
+
+    path = NodePath("node")
+
+    with pytest.raises(KeyError):
+        path.python_tags["foo"]
+
+    path.python_tags["foo"] = "bar"
+
+    assert path.python_tags["foo"] == "bar"
+
+    # Make sure reference count stays the same
+    rc1 = sys.getrefcount(path.python_tags)
+    rc2 = sys.getrefcount(path.python_tags)
+    assert rc1 == rc2