Comments (12)
bradallred
commented on June 11, 2024
This doesn't mean anything to me. memcheck probably isn't following the lifecycle into the python library. You can trivially setup a test to ensure these functions dont leak.
from gemrb.
MarcelHB
commented on June 11, 2024
I'll dig around. I just never noticed these report blocks before.
from gemrb.
lynxlynxlynx
commented on June 11, 2024
If it turns out indeed to be a dudd, we can add an exception — we have a suppression file in testing/python.supp (already not just for python).
from gemrb.
bradallred
commented on June 11, 2024
diff --git a/gemrb/plugins/GUIScript/PythonConversions.h b/gemrb/plugins/GUIScript/PythonConversions.h
index 423040683..89395c5e5 100644
--- a/gemrb/plugins/GUIScript/PythonConversions.h
+++ b/gemrb/plugins/GUIScript/PythonConversions.h
@@ -121,6 +121,7 @@ private:
static void PyRelease(PyObject *obj)
{
void* ptr = PyCapsule_GetPointer(obj, T::ID.description);
+ assert(ptr);
delete static_cast<Capsule*>(ptr);
}
that should be enough to convince us the CObject code doesn't leak in this path...
so then any leak would be due to a missing Py_DECREF: CObject creates a new reference via PyCapsule_New, we then Py_INCREF via operator PyObject*, giving us a ref count of two. You can try removing the Py_INCREF there and see if memcheck is appeased. However, that isn't the correct thing for us to do since it's possible to use the same CObject twice, and both ought to own their own reference. Maybe add a method to just get pycap for the places we return with one.
It's hard to deduce where all the locations operator PyObject* is getting invoked, I guess add explicit to it and weed your way though.
from gemrb.
MarcelHB
commented on June 11, 2024
I tried assert(false) there instead so if this method was called I should notice. Nothing happened.
I will look around and look at the ref counting.
from gemrb.
MarcelHB
commented on June 11, 2024
So this makes everything smooth (as far as I have tested):
diff --git a/gemrb/plugins/GUIScript/PythonConversions.h b/gemrb/plugins/GUIScript/PythonConversions.h
index 423040683..82833b526 100644
--- a/gemrb/plugins/GUIScript/PythonConversions.h
+++ b/gemrb/plugins/GUIScript/PythonConversions.h
@@ -110,6 +110,7 @@ public:
PyObject* kwargs = Py_BuildValue("{s:O}", "ID", obj);
pycap = gs->ConstructObject(T::ID.description, nullptr, kwargs);
Py_DECREF(kwargs);
+ Py_DECREF(obj);
}
}
That obj should continue to live for the time of pycap then.
from gemrb.
bradallred
commented on June 11, 2024
that doesn't seem right to me. How did you come to this conclusion?
ConstructObject should be returning an object with a ref count of 1 so this should be freeing that object entirely. It is also clear from what I've outlined above that we have a problem with operator PyObject* (which should be made explicit to prevent accidents like this).
from gemrb.
MarcelHB
commented on June 11, 2024
that doesn't seem right to me. How did you come to this conclusion?
PyCapsule_New -> 1, Py_BuildValue -> 2 and by that construction its likely to end up somewhere in Python's hold-graph over that object and thus my low-hanging-fruit assumption was that we have one undisposed reference to obj left here.
from gemrb.
bradallred
commented on June 11, 2024
Ah, I see. I thought CObject was hanging onto the capsule directly.
We should be using {s:N} for the format to Py_BuildValue and we may have made that mistake elsewhere too.
from gemrb.
bradallred
commented on June 11, 2024
could you try this:
diff --git a/gemrb/plugins/GUIScript/GUIScript.cpp b/gemrb/plugins/GUIScript/GUIScript.cpp
index ef0666f3d..81f409d5a 100644
--- a/gemrb/plugins/GUIScript/GUIScript.cpp
+++ b/gemrb/plugins/GUIScript/GUIScript.cpp
@@ -612,8 +612,8 @@ static PyObject* GemRB_LoadTable(PyObject * /*self*/, PyObject* args)
return RuntimeError("Can't find resource");
}
}
-
- return CObject<TableMgr, std::shared_ptr>(tab);
+
+ return PyObject_FromHolder<TableMgr>(tab);
}
PyDoc_STRVAR( GemRB_Table_GetValue__doc,
@@ -4564,7 +4564,7 @@ static PyObject* GemRB_SaveGame_GetPreview(PyObject * /*self*/, PyObject* args)
PARSE_ARGS( args, "O", &Slot );
Holder<SaveGame> save = CObject<SaveGame>(Slot);
- return CObject<Sprite2D>(save->GetPreview());
+ return PyObject_FromHolder<Sprite2D>(save->GetPreview());
}
PyDoc_STRVAR( GemRB_SaveGame_GetPortrait__doc,
@@ -4590,7 +4590,7 @@ static PyObject* GemRB_SaveGame_GetPortrait(PyObject * /*self*/, PyObject* args)
PARSE_ARGS( args, "Oi", &Slot, &index );
Holder<SaveGame> save = CObject<SaveGame>(Slot);
- return CObject<Sprite2D>(save->GetPortrait(index));
+ return PyObject_FromHolder<Sprite2D>(save->GetPortrait(index));
}
PyDoc_STRVAR( GemRB_GetGamePreview__doc,
@@ -6049,9 +6049,8 @@ static PyObject* GemRB_GetPlayerPortrait(PyObject * /*self*/, PyObject* args)
const Actor* actor = game->FindPC(PartyID);
if (actor) {
Holder<Sprite2D> portrait = actor->CopyPortrait(which);
- CObject<Sprite2D> obj(std::move(portrait));
PyObject* dict = PyDict_New();
- PyDict_SetItemString(dict, "Sprite", obj);
+ PyDict_SetItemString(dict, "Sprite", PyObject_FromHolder(portrait));
PyObject* pystr = PyString_FromResRef(which ? actor->SmallPortrait : actor->LargePortrait);
PyDict_SetItemString(dict, "ResRef", pystr);
Py_DecRef(pystr);
diff --git a/gemrb/plugins/GUIScript/PythonConversions.h b/gemrb/plugins/GUIScript/PythonConversions.h
index 423040683..0704af728 100644
--- a/gemrb/plugins/GUIScript/PythonConversions.h
+++ b/gemrb/plugins/GUIScript/PythonConversions.h
@@ -59,7 +59,7 @@ class CObject final {
public:
using CAP_T = PTR<T>;
- operator PyObject* () const
+ explicit operator PyObject* () const
{
if (pycap) {
Py_INCREF(pycap);
@@ -107,7 +107,7 @@ public:
cap = newcap;
}
- PyObject* kwargs = Py_BuildValue("{s:O}", "ID", obj);
+ PyObject* kwargs = Py_BuildValue("{s:N}", "ID", obj);
pycap = gs->ConstructObject(T::ID.description, nullptr, kwargs);
Py_DECREF(kwargs);
}
@@ -121,6 +121,7 @@ private:
static void PyRelease(PyObject *obj)
{
void* ptr = PyCapsule_GetPointer(obj, T::ID.description);
+ assert(ptr);
delete static_cast<Capsule*>(ptr);
}
@@ -247,17 +248,11 @@ PyObject* PyString_FromASCII(const STR& str)
// PyUnicode_FromStringAndSize expects UTF-8 data, ascii is compatible with that
return PyUnicode_FromStringAndSize(str.c_str(), str.length());
}
-
-template <typename T>
-PyObject* PyObject_FromPtr(T* p)
-{
- return CObject<T>(p);
-}
template <typename T>
PyObject* PyObject_FromHolder(Holder<T> h)
{
- return CObject<T>(std::move(h));
+ return static_cast<PyObject*>(CObject<T>(std::move(h)));
}
template <typename T, PyObject* (*F)(T), class Container>from gemrb.
MarcelHB
commented on June 11, 2024
Looks clean.
from gemrb.
bradallred
commented on June 11, 2024
great, thank you.
from gemrb.
Related Issues (20)
- BG2: web catches characters it shouldn't be able to HOT 13
- BG & OpenAL: Speech handling broken in CharGen in some circumstances HOT 2
- PST: text floats away too fast HOT 3
- Projectiles/Spell effects burning down too fast on higher FPS HOT 7
- Specialist mages saving throw modifiers don't work HOT 2
- LISTENER_HEIGHT increase results in too low volume for soundeffects HOT 8
- Should dialog volume actually be depending on listener position HOT 9
- `CreateItem`/`ci` console cheat command needs better item placement HOT 2
- Skull trap visual bugs HOT 9
- Wrong blending for some damage types HOT 8
- Some blending doesn't look good HOT 13
- Check new pc kit values
- Consider adding pathfinding tests
- BG1/BG2: some class restrictions don't apply for some races HOT 4
- BG2: Segmentation fault fighting some Goblins HOT 28
- Pointing on PCs as action target should also highlight their portrait HOT 3
- Crash in Promenade cutscene HOT 1
- Imoen doesn't dump her inventory in the Promenade - GivePartyAllEquipment HOT 1
- Full expanded UI option HOT 29
- Improve widescreen mod's selected resolution detection HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gemrb.