Memory Leak with PyUnicode_FromString() and PyDict_Clear

I’m looking for help, support on how to improve or eventually debug this particular code.
This is part of a large C++ application which allow to call python code.
I have tested on 3.9, 3.11 and 3.12 and when running valgrind , this report an important leak of memory in this CDevice_refresh().

I’m not the developper of this code, and not a specialist of the embedded python, so I’m looking for help and advice.

Is that the right way to do it ?
I feel that PyDict_Clear(self->Options) is cleaning the dictionnary ( self->Options), but in case anything has been allocated , it won’t be release. Am I wrong ? Is there a better way to write that ?

	PyObject* CDevice_refresh(CDevice* self)
	{
		if ((self->pPlugin) && (self->HwdID != -1) && (self->Unit != -1))
		{
			// load associated devices to make them available to python
			std::vector<std::vector<std::string> > result;
			result = m_sql.safe_query("SELECT Unit, ID, Name, nValue, sValue, DeviceID, Type, SubType, SwitchType, LastLevel, CustomImage, SignalLevel, BatteryLevel, LastUpdate, Options, Description, Color, Used FROM DeviceStatus WHERE (HardwareID==%d) AND (Unit==%d) ORDER BY Unit ASC", self->HwdID, self->Unit);
			if (!result.empty())
			{
				for (const auto &sd : result)
				{
					self->Unit = atoi(sd[0].c_str());
					self->ID = atoi(sd[1].c_str());
					Py_XDECREF(self->Name);
					self->Name = PyUnicode_FromString(sd[2].c_str());
					self->nValue = atoi(sd[3].c_str());
					Py_XDECREF(self->sValue);
					self->sValue = PyUnicode_FromString(sd[4].c_str());
					Py_XDECREF(self->DeviceID);
					self->DeviceID = PyUnicode_FromString(sd[5].c_str());
					self->Type = atoi(sd[6].c_str());
					self->SubType = atoi(sd[7].c_str());
					self->SwitchType = atoi(sd[8].c_str());
					self->LastLevel = atoi(sd[9].c_str());
					self->Image = atoi(sd[10].c_str());
					self->SignalLevel = atoi(sd[11].c_str());
					self->BatteryLevel = atoi(sd[12].c_str());
					Py_XDECREF(self->LastUpdate);
					self->LastUpdate = PyUnicode_FromString(sd[13].c_str());
					PyDict_Clear(self->Options);
					if (!sd[14].empty())
					{
						if (self->SubType == sTypeCustom)
						{
							PyDict_SetItemString(self->Options, "Custom", PyUnicode_FromString(sd[14].c_str()));
						}
						else
						{
							std::map<std::string, std::string> mpOptions;
							Py_BEGIN_ALLOW_THREADS
							mpOptions =	m_sql.BuildDeviceOptions(sd[14], true);
							Py_END_ALLOW_THREADS
							for (const auto &opt : mpOptions)
							{
								PyNewRef	pKeyDict = PyUnicode_FromString(opt.first.c_str());
								PyNewRef	pValueDict = PyUnicode_FromString(opt.second.c_str());
								if (PyDict_SetItem(self->Options, pKeyDict, pValueDict) == -1)
								{
									_log.Log(LOG_ERROR, "(%s) Failed to refresh Options dictionary for Hardware/Unit combination (%d:%d).", self->pPlugin->m_Name.c_str(), self->HwdID, self->Unit);
									break;
								}
							}
						}
					}
					Py_XDECREF(self->Description);
					self->Description = PyUnicode_FromString(sd[15].c_str());
					Py_XDECREF(self->Color);
					self->Color = PyUnicode_FromString(_tColor(std::string(sd[16])).toJSONString().c_str()); //Parse the color to detect incorrectly formatted color data
					self->Used = atoi(sd[17].c_str());
				}
			}
		}
		else
		{
			_log.Log(LOG_ERROR, "Device refresh failed, Device object is not associated with a plugin.");
		}

		Py_RETURN_NONE;
	}

Please post what valgrind reported.

Why is the title about PyUnicode_FromString but you go on to talk about PyDict_Clear?

I did update, I believe PyUnicode_FromString() if you don’t do Py_XDECREF() does also memory leak

for just the purpose of the test, I have commented all PyUnicode_FromString() and Py_XDECREF() as well as the PyDict_Clean() calls, and there is no memory leakage any more.

So what is the best practice to avoid such leakage

In this line:

PyDict_SetItemString(self->Options, "Custom", PyUnicode_FromString(sd[14].c_str()));

PyUnicode_FromString returns a new reference, then PyDict_SetItemString stores that reference and increfs, but the string isn’t decrefed.

Suppose that PyUnicode_FromString returned a new string. Its refcount would be1.

PyDict_SetItemString would store that string and increment the refcount to 2.

If self->Options was cleared or garbage collected, all the objects it referred to would be decrefed, so the string’s refcount would become 1.

But there are no other references to that string.

That’s a memory leak.

1 Like

prior to the PyDict_Clean() I have added the following, to release any reference if any, but still there is leak somewhere.

if (self->Options)
{
    Py_ssize_t pos = 0;
    PyObject *key, *value;

    while (PyDict_Next(self->Options, &pos, &key, &value))
    {
        // Decrement the reference count for each key and value in the dictionary
        Py_XDECREF(key);
        Py_XDECREF(value);
    }
}

When PyDict_Clear() clears the dict, it’ll decrefing the objects it refers to as it goes.

What you’re doing there is decrefing what will be decrefed anyway. Don’t do that. At a later time it’ll decref to 0 early and garbage collect the object, leading to a dangling pointer.

You should make the string, add it to the dict, and then decref the string:

PyObject* str = PyUnicode_FromString(sd[14].c_str());
PyDict_SetItemString(self->Options, "Custom", str);
Py_DECREF(str);
1 Like

The valgrind our available here on a github issue

Thanks, am I right in assuming @MRAB provided the solution?

Unfortunatly not.
I took @MRAB input, but I think we were not going in that code , so the issue is not there

CUnitEx_refresh looks OK to me, after the fix I suggested, but I’m no sure about CUnitEx_insert.

File domoticz/hardware/plugins/PythonObjectEx.cpp, line 675-681:

// otherwise update the parent if required
PyBorrowedRef pParent = PyDict_GetItem((PyObject *)pModState->pPlugin->m_DeviceDict, pDevice->DeviceID);
if (self->Parent != pParent)
{
	Py_DECREF(self->Parent);
	self->Parent = pParent;
}

pParent returns a borrowed reference, which you store in self->Parent, but self->Parent is decrefed when the parent object is deleted, so shouldn’t self->Parent be increfed?

Additional:

File domoticz/hardware/plugins/PythonObjectEx.cpp, line 921.

CUnitEx_touch returns 0 (nullptr), which signifies an error has occurred, but it looks like a Python error isn’t set.

On line 915, on the other hand, returns None.

So, “illegal operation, Plugin has not started yet.” is an error and “Unable to obtain module state.” is not? Yet both are logged as errors?

And here are some more leaks, the first one being in a loop!

File domoticz/main/EventsPythonModule.cpp, line 470-471:

PyDict_SetItemString(userVariablesDict, uvitem.variableName.c_str(),
	PyUnicode_FromString(uvitem.variableValue.c_str()));

File domoticz/hardware/plugins/PythonObjects.cpp, line 484-487:

PyDict_SetItemString(OptionsOut, "LevelActions", PyUnicode_FromString("|||"));
PyDict_SetItemString(OptionsOut, "LevelNames", PyUnicode_FromString("Off|Level1|Level2|Level3"));
PyDict_SetItemString(OptionsOut, "LevelOffHidden", PyUnicode_FromString("false"));
PyDict_SetItemString(OptionsOut, "SelectorStyle", PyUnicode_FromString("0"));

File domoticz/hardware/plugins/PythonObjects.cpp, line 528:

PyDict_SetItemString(OptionsOut, "Custom", PyUnicode_FromString("1"));

Thanks guys. I’ll work on that it is already great

Would like to come back on my memory leak issue.
I think with your support I have been able to identify real leak and fix them? That is good

However, I continue to have a "kind’ of memory leakage, in the sense that the memory seems to be never free.

What I would like to investigate is the following, and may be I’m wrong -as I’m not familiar with the Python in embedded C++ -

  • What triggers the garbage collector to start its work ?
  • If the program is spending most of its time in doing allocation/free, how can the garbage collector get a chance to do its work ?

My plan is to see if I can disable the GC from the C++ code (or eventually from the python plugin). Then see if I get a different behaviour
Then what is the impact when doing gc.collect() from the python code .

Question when I’m running gc.collect() inside a python code, will it force gc on all python environments running in the C++ code despite the number of thread and instance of python instances I’m running ?

It is rare that you will ever have to run the gc.collect() your self.
Because python uses reference counting the gc may have nothing to collect.

Once memory has been allocated by malloc and then free’ed it is only in rare situations that it will be returned to the OS.
Those situations depend on the details of the malloc implementation.
Assume small (for some definition of small) will never be returned.
Large can be returned to the OS on free.

I agree with the fact that memory is not return, but in my case I only see the Resident set size as well as the Virtual Memory of the process ( embedded python in C++) which is only growing minutes after minutes.
This is why I’m thinking of the GC not doing properly its jobs and not returning the memory to the process itself

The GC works very well, I would be very surprised if that is the source of your issues.

Does valgrind report memory leaks still?

yes still

==7925== 38,456 bytes in 253 blocks are indirectly lost in loss record 169 of 173
==7925==    at 0x48C0860: malloc (vg_replace_malloc.c:442)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:101)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:586)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:2003)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:1996)
==7925==    by 0x740762B: PyObject_Malloc (obmalloc.c:712)
==7925==    by 0x740F313: _PyType_AllocNoTrack (typeobject.c:1124)
==7925==    by 0x740F267: PyType_GenericAlloc (typeobject.c:1148)
==7925==    by 0xA4942B: Plugins::CDevice_new(_typeobject*, _object*, _object*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x741B74B: type_call (typeobject.c:1091)
==7925==    by 0x7457113: _PyObject_Call (call.c:343)
==7925==    by 0xA14B1F: Plugins::CPlugin::Start() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA1D8BB: Plugins::onStartCallback::ProcessLocked(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x5E2AD3: Plugins::CPluginMessageBase::Process(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA19C6F: Plugins::CPlugin::Do_Work() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xBC975F: execute_native_thread_routine (in /home/domoticz/dev-domoticz/domoticz)
==7925==
==7925== 139,295 (18,216 direct, 121,079 indirect) bytes in 253 blocks are definitely lost in loss record 170 of 173
==7925==    at 0x48C0860: malloc (vg_replace_malloc.c:442)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:101)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:586)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:2003)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:1996)
==7925==    by 0x740762B: PyObject_Malloc (obmalloc.c:712)
==7925==    by 0x74073CF: UnknownInlinedFun (gcmodule.c:2283)
==7925==    by 0x74073CF: _PyObject_GC_New (gcmodule.c:2298)
==7925==    by 0x740ED5B: PyCMethod_New (methodobject.c:101)
==7925==    by 0x743DE43: _PyObject_GenericGetAttrWithDict (object.c:1338)
==7925==    by 0x741EE0F: UnknownInlinedFun (object.c:1368)
==7925==    by 0x741EE0F: PyObject_GetAttr (object.c:916)
==7925==    by 0x744DD5B: PyObject_GetAttrString (object.c:801)
==7925==    by 0xA14B87: Plugins::CPlugin::Start() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA1D8BB: Plugins::onStartCallback::ProcessLocked(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x5E2AD3: Plugins::CPluginMessageBase::Process(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA19C6F: Plugins::CPlugin::Do_Work() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xBC975F: execute_native_thread_routine (in /home/domoticz/dev-domoticz/domoticz)
==7925==
==7925== 447,397 bytes in 7,583 blocks are definitely lost in loss record 171 of 173
==7925==    at 0x48C0860: malloc (vg_replace_malloc.c:442)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:101)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:586)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:2003)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:1996)
==7925==    by 0x740762B: PyObject_Malloc (obmalloc.c:712)
==7925==    by 0x7408EAF: PyUnicode_New (unicodeobject.c:1425)
==7925==    by 0x74081AF: unicode_decode_utf8 (unicodeobject.c:5117)
==7925==    by 0xA4C6CB: Plugins::CDevice_refresh(Plugins::CDevice*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x742E12F: _PyEval_EvalFrameDefault (ceval.c:5284)
==7925==    by 0x7421757: UnknownInlinedFun (pycore_ceval.h:73)
==7925==    by 0x7421757: _PyEval_Vector (ceval.c:6425)
==7925==    by 0x74180A3: _PyObject_VectorcallTstate.lto_priv.3 (pycore_call.h:92)
==7925==    by 0xA137E7: Plugins::CPlugin::Callback(Plugins::PyBorrowedRef&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _object*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x5E2AD3: Plugins::CPluginMessageBase::Process(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA19C6F: Plugins::CPlugin::Do_Work() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xBC975F: execute_native_thread_routine (in /home/domoticz/dev-domoticz/domoticz)
==7925==
==7925== 515,576 bytes in 7,582 blocks are definitely lost in loss record 172 of 173
==7925==    at 0x48C0860: malloc (vg_replace_malloc.c:442)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:101)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:586)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:2003)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:1996)
==7925==    by 0x740762B: PyObject_Malloc (obmalloc.c:712)
==7925==    by 0x7408EAF: PyUnicode_New (unicodeobject.c:1425)
==7925==    by 0x74081AF: unicode_decode_utf8 (unicodeobject.c:5117)
==7925==    by 0xA4C79F: Plugins::CDevice_refresh(Plugins::CDevice*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x742E12F: _PyEval_EvalFrameDefault (ceval.c:5284)
==7925==    by 0x7421757: UnknownInlinedFun (pycore_ceval.h:73)
==7925==    by 0x7421757: _PyEval_Vector (ceval.c:6425)
==7925==    by 0x74180A3: _PyObject_VectorcallTstate.lto_priv.3 (pycore_call.h:92)
==7925==    by 0xA137E7: Plugins::CPlugin::Callback(Plugins::PyBorrowedRef&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _object*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x5E2AD3: Plugins::CPluginMessageBase::Process(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA19C6F: Plugins::CPlugin::Do_Work() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xBC975F: execute_native_thread_routine (in /home/domoticz/dev-domoticz/domoticz)
==7925==
==7925== 580,513 bytes in 7,581 blocks are definitely lost in loss record 173 of 173
==7925==    at 0x48C0860: malloc (vg_replace_malloc.c:442)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:101)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:586)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:2003)
==7925==    by 0x740762B: UnknownInlinedFun (obmalloc.c:1996)
==7925==    by 0x740762B: PyObject_Malloc (obmalloc.c:712)
==7925==    by 0x7408EAF: PyUnicode_New (unicodeobject.c:1425)
==7925==    by 0x74081AF: unicode_decode_utf8 (unicodeobject.c:5117)
==7925==    by 0xA4C65B: Plugins::CDevice_refresh(Plugins::CDevice*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x742E12F: _PyEval_EvalFrameDefault (ceval.c:5284)
==7925==    by 0x7421757: UnknownInlinedFun (pycore_ceval.h:73)
==7925==    by 0x7421757: _PyEval_Vector (ceval.c:6425)
==7925==    by 0x74180A3: _PyObject_VectorcallTstate.lto_priv.3 (pycore_call.h:92)
==7925==    by 0xA137E7: Plugins::CPlugin::Callback(Plugins::PyBorrowedRef&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _object*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0x5E2AD3: Plugins::CPluginMessageBase::Process(Plugins::CPlugin*) (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xA19C6F: Plugins::CPlugin::Do_Work() (in /home/domoticz/dev-domoticz/domoticz)
==7925==    by 0xBC975F: execute_native_thread_routine (in /home/domoticz/dev-domoticz/domoticz)
==7925==

I would investigate the code in plugins::CPlugin::Start() and plugins::CDevice_refresh().

Leaks in extensions are usually caused by not calling decref when you no longer need an object. (And if you decref too many times, Python will crash at some point due to a hanging pointer.)

Another possibility is memory fragmentation because it doesn’t perform memory compaction.

But, as I said, the problem is usually getting the refcounting wrong.