Segfault fix in cpp_function::dispatcher of an overloaded function with *args/**kwargs #1223
Conversation
…nction with args/kwargs, during the second pass over the overloads (with conversions). Inside the overload loop, new `py::object`s are created for the *args and **kwargs arguments (`extra_args` and `kwargs`), but these go out of scope by the time a second pass with conversions over the different overloads is executed. At the destruction of the `argument_loader`, its nested casters, and the `py::args` object, if Python already garbage collected, things go wrong. A quick, 3-line solution is to keep a `std::vector<py::object>` and add the `extra_args` and `kwargs` instances to keep them alive for long enough.
|
I can't look into this in detail right now, but my initial thought (which may well be wrong) is that it seems as though they should be kept alive by being inside the |
|
As far as I saw, |
|
Ah yes, I see. (I forgot: we wanted a handle vector there to avoid a bunch of unnecessary inc_ref/dec_refs in here). |
The `py::args` or `py::kwargs` arguments aren't properly referenced when added to the function_call arguments list: their reference counts drop to zero if the first (non-converting) function call fails, which means they might be cleaned up before the second pass call runs. This commit adds a couple of extra `object`s to the `function_call` where we can stash a reference to them when needed to tie their lifetime to the function_call object's lifetime. (Credit to YannickJadoul for catching and proposing a fix in pybind#1223).
|
I agree that we need another keep-alive reference here, but I don't really like the idea of introducing another vector here—we'd be introducing extra mallocs into the function dispatcher (which is performance-critical code). I think putting the extra keepalive Try out 7c306b1 -- I think that should solve it. |
|
Ok, agreed, that's a fine solution too. And the error in the specific case I had is also solved by it (as I just tested), so ... thanks! |
|
(I'm going to keep this open until I merge that commit into master just to keep it on my radar). |
The `py::args` or `py::kwargs` arguments aren't properly referenced when added to the function_call arguments list: their reference counts drop to zero if the first (non-converting) function call fails, which means they might be cleaned up before the second pass call runs. This commit adds a couple of extra `object`s to the `function_call` where we can stash a reference to them when needed to tie their lifetime to the function_call object's lifetime. (Credit to YannickJadoul for catching and proposing a fix in pybind#1223).
The `py::args` or `py::kwargs` arguments aren't properly referenced when added to the function_call arguments list: their reference counts drop to zero if the first (non-converting) function call fails, which means they might be cleaned up before the second pass call runs. This commit adds a couple of extra `object`s to the `function_call` where we can stash a reference to them when needed to tie their lifetime to the function_call object's lifetime. (Credit to YannickJadoul for catching and proposing a fix in #1223).
|
Merged the fix into master (48e1f9a). |
|
Thanks for the quick fix! |
The `py::args` or `py::kwargs` arguments aren't properly referenced when added to the function_call arguments list: their reference counts drop to zero if the first (non-converting) function call fails, which means they might be cleaned up before the second pass call runs. This commit adds a couple of extra `object`s to the `function_call` where we can stash a reference to them when needed to tie their lifetime to the function_call object's lifetime. (Credit to YannickJadoul for catching and proposing a fix in pybind#1223).
The `py::args` or `py::kwargs` arguments aren't properly referenced when added to the function_call arguments list: their reference counts drop to zero if the first (non-converting) function call fails, which means they might be cleaned up before the second pass call runs. This commit adds a couple of extra `object`s to the `function_call` where we can stash a reference to them when needed to tie their lifetime to the function_call object's lifetime. (Credit to YannickJadoul for catching and proposing a fix in #1223).
Fixing a segfault in the cpp_function::dispatcher of an overloaded function with args/kwargs, during the second pass over the overloads (with conversions).
Inside the overload loop, new
py::objects are created for the *args and **kwargs arguments (extra_argsandkwargs), but these go out of scope by the time a second pass with conversions over the different overloads is executed.At the destruction of the
argument_loader, its nested casters, and thepy::argsobject, if Python already garbage collected, things go wrong.A quick, 3-line solution is to keep a
std::vector<py::object>and add theextra_argsandkwargsinstances to keep them alive for long enough.Attached is a trace of the segfault in GDB (running
python3-gdb;python3just gets corrupted instead of segfaulting and shoutsFatal Python error: GC object already trackedsomewhere later):args-crash-gdb-bt.txt
I don't know if there might be a better/cleaner/... solution than this 'keepalive' vector, but this at least seems to be solving the segfaults/corruption and does not change the flow of the dispatcher.