[C++-sig] Segfault with keyword arguments and overloads.

[Alex Leach]

Hi,

On Wed, 07 Aug 2013 01:35:54 +0100, Alex Mohr <amohr at pixar.com> wrote:

> I'm hitting a repeatable segfault trying to upgrade our boost version. I  
> believe there may be a bug with overload resolution in the presence of  
> keyword arguments.  Here's a minimal example that crashes for me.
>
> // C++
> #include <boost/python.hpp>
>
> static void f1(int a0, int a1) { }
> static void f2(int a0, int a1, int a2) { }
>
> BOOST_PYTHON_MODULE(kwargCrash) {
>      boost::python::def("f", f1, (arg("a1")=2));
>      boost::python::def("f", f2, (arg("a2")=2));
> }


I think the signature looks incorrect here. From my understanding of  
signatures, if you define one argument name, you should define the name  
for all arguments. On class methods, this includes adding an arg for  
'self'.

See  
http://www.boost.org/doc/libs/1_54_0/libs/python/doc/v2/function_doc_signature.html

Also, it seems strange that you're assigning the final argument a default  
value, when the actual C++ function has none. I did add the corresponding  
default argument to the C++ function signatures, but that still seg  
faults, so I guess you've got default arguments in your actual code (good  
minimal example, though!). However, the function signature in the  
documentation - ie. from `help(kwargCrash)` - looks correct.

Still, the following code makes the documentation more complete, and also  
it actually works:-

#include <boost/python/module.hpp>
#include <boost/python/args.hpp>
#include <boost/python/def.hpp>

static void f1(int a0, int a1=2) { }
static void f2(int a0, int a1, int a2=2) { }

BOOST_PYTHON_MODULE(kwargCrash) {
     boost::python::def("f", f1, (boost::python::arg("a0"),
                                  boost::python::arg("a1")=2));
     boost::python::def("f", f2, (boost::python::arg("a0"),
                                  boost::python::arg("a1"),
                                  boost::python::arg("a2")=2));
}




>
> # Python
> import kwargCrash
> kwargCrash.f(0, a1=2)
>
> Version info: boost 1.51.0, Python 2.7.5, gcc 4.8.1.
>
> In the example we pass one positional argument and one keyword argument.  
>   This should fail to match the second overload (f2) since two  
> positional arguments are required, and should match and invoke the first  
> (f1).  Instead, it segfaults.
>
> Here's my hypothesis as to what's happening from inspecting the  
> boost.python code:
>
> In libs/python/src/object/function.cpp, function::call() (line 123)
>    - We consider overloads in reverse reg order, so we try 'f2' first.
>    - We pass the if on line 138, since n_actual is 2,  
> f->m_nkeyword_values is 1, and min/max_arity==3.
>    - The if on line 144 passes, since kwargs were supplied.
>    - The fn has kwargs, so we take the 'else' on line 152.
>    - We reach the 'build a new arg tuple...' else clause on line 167.
>    - We make a tuple of size 3 and set the first element with the  
> positional arg '0'.
>    - Now we loop arg_pos = 1 to max_arity-1 (line 180).
>    - Line 183, we set: kv = PyTuple_GET_ITEM(f->m_arg_names, arg_pos).
>    - Line 189, we do: PyTuple_GET_ITEM(kv, 0).
>
> In this case, I believe kv is actually None, not a tuple, which gives us  
> a bogus result, and I crash in the PyDict_GetItem call.  Here's why I  
> believe kv is actually None:
>
> In function.cpp again, function ctor (line 67)
>    - Consider when an instance is created for 'f2'.
>    - Line 72: keyword_offset gets set to 2 (max_arity = 3, num_keywords  
> = 1).
>    - m_arg_names is created with size 3 (max_arity)
>    - Line 80: loop and set the first 2 elements (keyword_offset) to None.
>
> So the leading elements of the tuple get set to None, until we actually  
> have keywords -- those elements get set to pairs of kwarg-name,  
> kwarg-value.
>
> If my inspection is valid, then the indexing logic in function::call is  
> busted.  It assumes that it always gets a tuple of name/value for kv if  
> it gets to line 189.  But in this case it gets None since the argument  
> at index 1 has no keyword.



True, the first arguments have no signature. I guess it was assumed that  
if you supply one argument's signature, you would supply them all. Looking  
at the docs for PyTuple_GET_ITEM, it is the same as PyTuple_GetItem,  
except that no checking is performed. So it probably returns NULL, which  
in turn causes PyDict_GetItem to seg fault. Here's my take, in the form of  
a working patch:


---  
/home/albl500/builds/boost/src/boost_1_53_0/libs/python/src/object/function.cpp.orig     
2013-08-07 11:02:28.569236835 +0100
+++  
/home/albl500/builds/boost/src/boost_1_53_0/libs/python/src/object/function.cpp  
2013-08-07 11:09:09.688030081 +0100
@@ -177,17 +177,19 @@
                          // Grab remaining arguments by name from the  
keyword dictionary
                          std::size_t n_actual_processed = n_unnamed_actual;

+                        PyObject *kv, *k, *value;
                          for (std::size_t arg_pos = n_unnamed_actual;  
arg_pos < max_arity ; ++arg_pos)
                          {
                              // Get the keyword[, value pair] corresponding
-                            PyObject* kv =  
PyTuple_GET_ITEM(f->m_arg_names.ptr(), arg_pos);
+                            kv = PyTuple_GET_ITEM(f->m_arg_names.ptr(),  
arg_pos);

                              // If there were any keyword arguments,
                              // look up the one we need for this
                              // argument position
-                            PyObject* value = n_keyword_actual
-                                ? PyDict_GetItem(keywords,  
PyTuple_GET_ITEM(kv, 0))
-                                : 0;
+                            k = PyTuple_GET_ITEM(kv, 0);
+                            value = (k != NULL && n_keyword_actual)
+                                    ? PyDict_GetItem(keywords, k)
+                                    : 0;

                              if (!value)
                              {

>
> Does this sound plausible?  I'm a bit surprised this was working for us  
> previously, as the code looks like it hasn't really changed.  Maybe we  
> were getting lucky and Python changed in a way that breaks us?  I.e.  
> maybe PyTuple_GET_ITEM actually gave None if kv was None, and we never  
> had None as a key in the passed keywords dict.

I don't know of any code changes that would have introduced this..


>
> Assuming my hypothesis is true, I'm not sure what the right fix here is  
> off the top of my head.  Naively it seems like checking for None in  
> m_arg_names and rejecting the overload might work, but it feels like the  
> right thing to do is to rework the logic used to decide whether or not a  
> call has a satisfactory combination of positional and keyword args.
>
> Any thoughts appreciated.
>
> Alex

HTH. Cheers,
Alex