Fix a memory leak when creating Python3 modules.

[T045T]

With no segfaults, this time!

[bstaletic]

For my own understanding, what is module in the lambda? Is that m_ptr?
Also is the return value of PyModule_GetDef the def from a few lines above?

[bstaletic]

This can just be if (module). If you really want to be explicit in your bool expressions, use module != nullptr.

[bstaletic]

What's happening here? Why would the python allocator be trying to free memory that it has never allocated?

[T045T]

Sorry, this was me trying a theory on Travis, because I could not reproduce the failure locally even in Docker

[T045T]

(My theory was wrong, anyway)

[bstaletic]

Oh, I didn't notice the travis failure before. I can still repro, though I get the same error as travis, but on pytest, not cpptest.

Hop over to gitter. It will be easier to chat there and look into this.

[T045T]

Yes, you're right - module is m_ptr, but cast to void*, and PyModule_GetDef() returns def.

The issue here is that PyModule is a struct that's defined only in a .cc file, so we cannot cast module to PyModule* ourselves, and instead have to rely on PyModule_GetDef().

[wjakob]

I think you will need to memset() the new-allocated memory region to zero before running INCREF on it.

[bstaletic]

I think you will need to memset()

That would make memset() immediately follow new. That's perfectly fine, but that's exactly what calloc() does, so the two lines could be replaced with just calloc(1, sizeof(...)).

That property of calloc() is subtle, so I'll leave to you two to decide whether this is a good idea.

[wjakob]

Either variant is fine with me, let's just not mix calloc and delete :-)

[T045T]

This turned out to be more tricky than anticipated. The current version is not meant to be merged as-is, but to share my current status for you to see.

[wjakob]

Any news here? Is anyone interested in picking up the work on this minor leak (which I understand can be rather annoying in the context of valgrind).

[bstaletic]

Yeah, I've worked on this with @T045T for a while in December. One approach fixes one version of python, other fixes two other versions of python, yadda yadda yadda...

It seems like CPython API expects this thing to have static lifetime and different versions of python rely on that assumption in different ways. I don't think we can solve this leak by a cleverly placed delete and not be at risk of it breaking when the next version of python comes out. Or the version after that.

Maybe valgrind users should just accept the 104*interpreter_finalization_calls bytes being leaked and write a suppression rule.

Here's the suppression rule for python 3.6 compiled in debug mode:

{
	Pybind11 leak, see https://github.com/pybind/pybind11/pull/2024 - definitely lost
	Memcheck:Leak
	match-leak-kinds: definite
	fun:_Znwm
	fun:_ZN8pybind116moduleC1EPKcS2_
	fun:PyInit_ycm_core
	fun:_PyImport_LoadDynamicModuleWithSpec
	fun:_imp_create_dynamic_impl
	fun:_imp_create_dynamic
	fun:cfunction_vectorcall_FASTCALL
	fun:PyVectorcall_Call
	fun:do_call_core
	fun:_PyEval_EvalFrameDefault
	fun:_PyEval_EvalCodeWithName
	fun:_PyFunction_Vectorcall
	fun:_PyObject_Vectorcall
	fun:call_function
	fun:_PyEval_EvalFrameDefault
	fun:function_code_fastcall
}

[bstaletic]

We likely want to use this thing.

From the Python extension module tutorial on defining types:

> What we’re showing here is the traditional way of defining static extension types. It should be adequate for most uses. The C API also allows defining heap-allocated extension types using the PyType_FromSpec() function, which isn’t covered in this tutorial.

EDIT: Forget what I've just said. I mixed something up.

[rwgk]

I just merged #2413 which solves the leak check issue in a different way. Closing this PR. Please let us know if you're still seeing leak check errors in your environment (seems very unlikely).