bug 4335 - Python segfault

6 files changed, 139 insertions, 73 deletions

diff --git a/py/modules/IcePy/Communicator.cpp b/py/modules/IcePy/Communicator.cpp
index 72f33ff56b5..f3cf4f3d4f7 100644
--- a/py/modules/IcePy/Communicator.cpp
+++ b/py/modules/IcePy/Communicator.cpp
@@ -476,12 +476,17 @@ static PyObject*
 communicatorProxyToString(CommunicatorObject* self, PyObject* args)
 {
     PyObject* obj;
-    if(!PyArg_ParseTuple(args, STRCAST("O!"), &ProxyType, &obj))
+    if(!PyArg_ParseTuple(args, STRCAST("O"), &obj))
+    {
+        return 0;
+    }
+
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(obj, "proxyToString", "obj", proxy))
     {
         return 0;
     }
 
-    Ice::ObjectPrx proxy = getProxy(obj);
     string str;
 
     assert(self->communicator);
@@ -542,9 +547,13 @@ extern "C"
 static PyObject*
 communicatorProxyToProperty(CommunicatorObject* self, PyObject* args)
 {
+    //
+    // We don't want to accept None here, so we can specify ProxyType and force
+    // the caller to supply a proxy object.
+    //
     PyObject* proxyObj;
     PyObject* strObj;
-    if(!PyArg_ParseTuple(args, STRCAST("OO"), &proxyObj, &strObj))
+    if(!PyArg_ParseTuple(args, STRCAST("O!O"), &ProxyType, &proxyObj, &strObj))
     {
         return 0;
     }
@@ -1071,8 +1080,8 @@ static PyObject*
 communicatorCreateObjectAdapterWithRouter(CommunicatorObject* self, PyObject* args)
 {
     PyObject* nameObj;
-    PyObject* proxy;
-    if(!PyArg_ParseTuple(args, STRCAST("OO"), &nameObj, &proxy))
+    PyObject* p;
+    if(!PyArg_ParseTuple(args, STRCAST("OO"), &nameObj, &p))
     {
         return 0;
     }
@@ -1083,25 +1092,19 @@ communicatorCreateObjectAdapterWithRouter(CommunicatorObject* self, PyObject* ar
         return 0;
     }
 
-    PyObject* routerProxyType = lookupType("Ice.RouterPrx");
-    assert(routerProxyType);
-    Ice::RouterPrx router;
-    if(PyObject_IsInstance(proxy, routerProxyType))
-    {
-        router = Ice::RouterPrx::uncheckedCast(getProxy(proxy));
-    }
-    else if(proxy != Py_None)
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "createObjectAdapterWithRouter", "rtr", proxy, "Ice.RouterPrx"))
     {
-        PyErr_Format(PyExc_ValueError, STRCAST("createObjectAdapterWithRouter requires None or Ice.RouterPrx"));
         return 0;
     }
 
-    AllowThreads allowThreads; // Release Python's global interpreter lock to avoid a potential deadlock.
+    Ice::RouterPrx router = Ice::RouterPrx::uncheckedCast(proxy);
 
     assert(self->communicator);
     Ice::ObjectAdapterPtr adapter;
     try
     {
+        AllowThreads allowThreads; // Release Python's global interpreter lock to avoid a potential deadlock.
         adapter = (*self->communicator)->createObjectAdapterWithRouter(name, router);
     }
     catch(const Ice::Exception& ex)
@@ -1160,25 +1163,20 @@ extern "C"
 static PyObject*
 communicatorSetDefaultRouter(CommunicatorObject* self, PyObject* args)
 {
-    PyObject* proxy;
-    if(!PyArg_ParseTuple(args, STRCAST("O"), &proxy))
+    PyObject* p;
+    if(!PyArg_ParseTuple(args, STRCAST("O"), &p))
     {
         return 0;
     }
 
-    PyObject* routerProxyType = lookupType("Ice.RouterPrx");
-    assert(routerProxyType);
-    Ice::RouterPrx router;
-    if(PyObject_IsInstance(proxy, routerProxyType))
-    {
-        router = Ice::RouterPrx::uncheckedCast(getProxy(proxy));
-    }
-    else if(proxy != Py_None)
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "setDefaultRouter", "rtr", proxy, "Ice.RouterPrx"))
     {
-        PyErr_Format(PyExc_ValueError, STRCAST("setDefaultRouter requires None or Ice.RouterPrx"));
         return 0;
     }
 
+    Ice::RouterPrx router = Ice::RouterPrx::uncheckedCast(proxy);
+
     assert(self->communicator);
     try
     {
@@ -1229,25 +1227,20 @@ extern "C"
 static PyObject*
 communicatorSetDefaultLocator(CommunicatorObject* self, PyObject* args)
 {
-    PyObject* proxy;
-    if(!PyArg_ParseTuple(args, STRCAST("O"), &proxy))
+    PyObject* p;
+    if(!PyArg_ParseTuple(args, STRCAST("O"), &p))
     {
         return 0;
     }
 
-    PyObject* locatorProxyType = lookupType("Ice.LocatorPrx");
-    assert(locatorProxyType);
-    Ice::LocatorPrx locator;
-    if(PyObject_IsInstance(proxy, locatorProxyType))
-    {
-        locator = Ice::LocatorPrx::uncheckedCast(getProxy(proxy));
-    }
-    else if(proxy != Py_None)
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "setDefaultLocator", "loc", proxy, "Ice.LocatorPrx"))
     {
-        PyErr_Format(PyExc_ValueError, STRCAST("setDefaultLocator requires None or Ice.LocatorPrx"));
         return 0;
     }
 
+    Ice::LocatorPrx locator = Ice::LocatorPrx::uncheckedCast(proxy);
+
     assert(self->communicator);
     try
     {
diff --git a/py/modules/IcePy/ObjectAdapter.cpp b/py/modules/IcePy/ObjectAdapter.cpp
index c5653e11bdc..e870654cf3c 100644
--- a/py/modules/IcePy/ObjectAdapter.cpp
+++ b/py/modules/IcePy/ObjectAdapter.cpp
@@ -1159,9 +1159,12 @@ extern "C"
 static PyObject*
 adapterFindByProxy(ObjectAdapterObject* self, PyObject* args)
 {
-    PyObject* proxyType = lookupType("Ice.ObjectPrx");
+    //
+    // We don't want to accept None here, so we can specify ProxyType and force
+    // the caller to supply a proxy object.
+    //
     PyObject* proxy;
-    if(!PyArg_ParseTuple(args, STRCAST("O!"), proxyType, &proxy))
+    if(!PyArg_ParseTuple(args, STRCAST("O!"), &ProxyType, &proxy))
     {
         return 0;
     }
@@ -1458,14 +1461,19 @@ extern "C"
 static PyObject*
 adapterSetLocator(ObjectAdapterObject* self, PyObject* args)
 {
-    PyObject* proxyType = lookupType("Ice.LocatorPrx");
-    PyObject* proxy;
-    if(!PyArg_ParseTuple(args, STRCAST("O!"), proxyType, &proxy))
+    PyObject* p;
+    if(!PyArg_ParseTuple(args, STRCAST("O"), &p))
+    {
+        return 0;
+    }
+
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "setLocator", "loc", proxy, "Ice.LocatorPrx"))
     {
         return 0;
     }
 
-    Ice::LocatorPrx locator = Ice::LocatorPrx::uncheckedCast(getProxy(proxy));
+    Ice::LocatorPrx locator = Ice::LocatorPrx::uncheckedCast(proxy);
 
     assert(self->adapter);
     try
diff --git a/py/modules/IcePy/Proxy.cpp b/py/modules/IcePy/Proxy.cpp
index da6e94d0b8b..36aea04dece 100644
--- a/py/modules/IcePy/Proxy.cpp
+++ b/py/modules/IcePy/Proxy.cpp
@@ -907,25 +907,20 @@ proxyIceRouter(ProxyObject* self, PyObject* args)
         return 0;
     }
 
-    PyObject* routerProxyType = lookupType("Ice.RouterPrx");
-    assert(routerProxyType);
-    Ice::RouterPrx routerProxy;
-    if(PyObject_IsInstance(p, routerProxyType))
-    {
-        routerProxy = Ice::RouterPrx::uncheckedCast(getProxy(p));
-    }
-    else if(p != Py_None)
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "ice_router", "rtr", proxy, "Ice.RouterPrx"))
     {
-        PyErr_Format(PyExc_ValueError, STRCAST("ice_router requires None or Ice.RouterPrx"));
         return 0;
     }
 
+    Ice::RouterPrx router = Ice::RouterPrx::uncheckedCast(proxy);
+
     assert(self->proxy);
 
     Ice::ObjectPrx newProxy;
     try
     {
-        newProxy = (*self->proxy)->ice_router(routerProxy);
+        newProxy = (*self->proxy)->ice_router(router);
     }
     catch(const Ice::Exception& ex)
     {
@@ -978,25 +973,20 @@ proxyIceLocator(ProxyObject* self, PyObject* args)
         return 0;
     }
 
-    PyObject* locatorProxyType = lookupType("Ice.LocatorPrx");
-    assert(locatorProxyType);
-    Ice::LocatorPrx locatorProxy;
-    if(PyObject_IsInstance(p, locatorProxyType))
+    Ice::ObjectPrx proxy;
+    if(!getProxyArg(p, "ice_locator", "loc", proxy, "Ice.LocatorPrx"))
     {
-        locatorProxy = Ice::LocatorPrx::uncheckedCast(getProxy(p));
-    }
-    else if(p != Py_None)
-    {
-        PyErr_Format(PyExc_ValueError, STRCAST("ice_locator requires None or Ice.LocatorPrx"));
         return 0;
     }
 
+    Ice::LocatorPrx locator = Ice::LocatorPrx::uncheckedCast(proxy);
+
     assert(self->proxy);
 
     Ice::ObjectPrx newProxy;
     try
     {
-        newProxy = (*self->proxy)->ice_locator(locatorProxy);
+        newProxy = (*self->proxy)->ice_locator(locator);
     }
     catch(const Ice::Exception& ex)
     {
@@ -2044,6 +2034,50 @@ IcePy::getProxy(PyObject* p)
     return *obj->proxy;
 }
 
+bool
+IcePy::getProxyArg(PyObject* p, const string& func, const string& arg, Ice::ObjectPrx& proxy, const string& type)
+{
+    bool result = true;
+
+    if(checkProxy(p))
+    {
+        if(!type.empty())
+        {
+            PyObject* proxyType = lookupType(type);
+            assert(proxyType);
+            if(!PyObject_IsInstance(p, proxyType))
+            {
+                result = false;
+            }
+        }
+    }
+    else if(p != Py_None)
+    {
+        result = false;
+    }
+
+    if(result)
+    {
+        if(p != Py_None)
+        {
+            ProxyObject* obj = reinterpret_cast<ProxyObject*>(p);
+            proxy = *obj->proxy;
+        }
+        else
+        {
+            proxy = 0;
+        }
+    }
+    else
+    {
+        string typeName = type.empty() ? "Ice.ObjectPrx" : type;
+        PyErr_Format(PyExc_ValueError, STRCAST("%s expects a proxy of type %s or None for argument '%s'"),
+                     func.c_str(), typeName.c_str(), arg.c_str());
+    }
+
+    return result;
+}
+
 Ice::CommunicatorPtr
 IcePy::getProxyCommunicator(PyObject* p)
 {
diff --git a/py/modules/IcePy/Proxy.h b/py/modules/IcePy/Proxy.h
index 0d79395f2c0..8b2bfc34765 100644
--- a/py/modules/IcePy/Proxy.h
+++ b/py/modules/IcePy/Proxy.h
@@ -23,9 +23,29 @@ bool initProxy(PyObject*);
 
 PyObject* createProxy(const Ice::ObjectPrx&, const Ice::CommunicatorPtr&, PyObject* = 0);
 
+//
+// Verifies that the given Python object is a proxy. A value of None is not considered legal here.
+//
 bool checkProxy(PyObject*);
 
+//
+// Extracts a proxy from the given Python object. The Python object *must* be a proxy.
+// None is not legal here.
+//
 Ice::ObjectPrx getProxy(PyObject*);
+
+//
+// Extracts a proxy argument from the given Python object. None is accepted here. If the Python
+// object contains an invalid value, the function raises a ValueError exception and returns
+// false. The optional trailing string provides the Python class name of a derived Slice
+// interface that the caller requires.
+//
+bool getProxyArg(PyObject*, const std::string&, const std::string&, Ice::ObjectPrx&,
+                 const std::string& = std::string());
+
+//
+// Gets the communicator associated with the proxy object.
+//
 Ice::CommunicatorPtr getProxyCommunicator(PyObject*);
 
 }
diff --git a/py/modules/IcePy/Util.cpp b/py/modules/IcePy/Util.cpp
index 08d294c660e..bc0101ad351 100644
--- a/py/modules/IcePy/Util.cpp
+++ b/py/modules/IcePy/Util.cpp
@@ -43,21 +43,27 @@ IcePy::getStringArg(PyObject* p, const string& arg, string& val)
     }
     else if(p != Py_None)
     {
-        //
-        // Get name of current function.
-        //
-        PyFrameObject *f = PyThreadState_GET()->frame;
-        PyObjectHandle code = PyObject_GetAttrString(reinterpret_cast<PyObject*>(f), STRCAST("f_code"));
-        assert(code.get());
-        PyObjectHandle func = PyObject_GetAttrString(code.get(), STRCAST("co_name"));
-        assert(func.get());
-        string funcName = getString(func.get());
+        string funcName = getFunction();
         PyErr_Format(PyExc_ValueError, STRCAST("%s expects a string for argument '%s'"), funcName.c_str(), arg.c_str());
         return false;
     }
     return true;
 }
 
+string
+IcePy::getFunction()
+{
+    //
+    // Get name of current function.
+    //
+    PyFrameObject *f = PyThreadState_GET()->frame;
+    PyObjectHandle code = PyObject_GetAttrString(reinterpret_cast<PyObject*>(f), STRCAST("f_code"));
+    assert(code.get());
+    PyObjectHandle func = PyObject_GetAttrString(code.get(), STRCAST("co_name"));
+    assert(func.get());
+    return getString(func.get());
+}
+
 IcePy::PyObjectHandle::PyObjectHandle(PyObject* p) :
     _p(p)
 {
diff --git a/py/modules/IcePy/Util.h b/py/modules/IcePy/Util.h
index 0780fd67f6b..5d860d246c8 100644
--- a/py/modules/IcePy/Util.h
+++ b/py/modules/IcePy/Util.h
@@ -60,11 +60,16 @@ inline PyObject* createString(const std::string& str)
 std::string getString(PyObject*);
 
 //
-// Validate and retrieve a string argument.
+// Validate and retrieve a string argument; None is also legal.
 //
 bool getStringArg(PyObject*, const std::string&, std::string&);
 
 //
+// Get the name of the current Python function.
+//
+std::string getFunction();
+
+//
 // Invokes Py_DECREF on a Python object.
 //
 class PyObjectHandle