Segfault in name mapper

Jon Siddle <[email protected]> Mon, 13 Sep 2010 12:00:06 +0100

Newsgroups	gmane.comp.python.cheetah
Organization	Corefiling Ltd
Message-ID	<[email protected]>

   It is possible to trivially cause the name mapper to segfault if 
getattr throws an exception:

$a.b.c

If a.__getattr__('b') throws an exception, the name mapper will 
segfault. This is caused by the
following code in PyNamemapper_valueForName():

nextVal = PyObject_GetAttrString(currentVal, currentKey);
if (exc != NULL) {
      // if exception == AttributeError
      if (PyErr_ExceptionMatches(PyExc_AttributeError)) {
          setNotFoundException(currentKey, currentVal);
          if (i > 0) {
              Py_DECREF(currentVal);
          }
          return NULL;
      }
}

As you can see, only AttributeError is handled. Any other exception will 
cause currentVal to be NULL
the next time round the loop and the call to 
PyObject_GetAttrString(currentVal, currentKey) will segfault.

The solution is simply to move the decref-return code outside the check 
for AttributeError so that it happens
for all exceptions.

The attached patch fixes the bug, and adds a test for it. I've added a 
return code check to the buildandrun
script so the segfault doesn't go unnoticed when the test fails.

FYI We encountered this when using the sqlalchemy ORM, which can throw 
exceptions from __getattr__ on
objects in the session, so it's quite likely that others will encounter 
this.

Thanks

Jon

-- 
Jon Siddle, CoreFiling Limited
Software Tools Developer
http://www.corefiling.com
Phone: +44-1865-203192

------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing
http://p.sf.net/sfu/novell-sfdev2dev

_______________________________________________
Cheetahtemplate-discuss mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/cheetahtemplate-discuss

getattr-segfault-fix.patch (text/plain, 2.4 KB)

diff --git a/buildandrun b/buildandrun
index d3b4e42..41a636a 100755
--- a/buildandrun
+++ b/buildandrun
@@ -58,7 +58,8 @@ def main():
     os.putenv('PYTHONPATH', libdir)
 
     rc = subprocess.call( ['python',] + args )
-
+    if rc == -11:
+        logging.error('Segmentation fault in test process. Test failed.')
 
 
 if __name__ == '__main__':
diff --git a/cheetah/Tests/NameMapper.py b/cheetah/Tests/NameMapper.py
index ac42244..c44fb4c 100644
--- a/cheetah/Tests/NameMapper.py
+++ b/cheetah/Tests/NameMapper.py
@@ -43,6 +43,10 @@ class DummyClass(object):
         except:
             raise
 
+class DummyClassGetAttrRaises(object):
+    def __getattr__(self, name):
+        raise ValueError
+
 
 def dummyFunc(arg="Scooby"):
     return arg
@@ -67,6 +71,7 @@ testNamespace = {
     'aClass': DummyClass,    
     'aFunc': dummyFunc,
     'anObj': DummyClass(),
+    'anObjThatRaises': DummyClassGetAttrRaises(),
     'aMeth': DummyClass().meth1,
     'none': None,  
     'emptyString': '',
@@ -419,6 +424,14 @@ class VFN(NameMapperTest):
         for i in range(10):
             self.get('aDict.nestedDict.funcThatRaises', False)    
 
+    def test61(self):
+        """Accessing attribute where __getattr__ raises shouldn't segfault if something follows it"""
+
+        def test(self=self):
+            self.get('anObjThatRaises.willraise.anything')
+        self.assertRaises(ValueError, test)
+
+
 class VFS(VFN):
     _searchListLength = 1
     
diff --git a/cheetah/c/_namemapper.c b/cheetah/c/_namemapper.c
index a41571b..a114658 100644
--- a/cheetah/c/_namemapper.c
+++ b/cheetah/c/_namemapper.c
@@ -188,14 +188,15 @@ static PyObject *PyNamemapper_valueForName(PyObject *obj, char *nameChunks[], in
           nextVal = PyObject_GetAttrString(currentVal, currentKey);
           exc = PyErr_Occurred();
           if (exc != NULL) {
-            // if exception == AttributeError
+            // if exception == AttributeError, report our own exception
             if (PyErr_ExceptionMatches(PyExc_AttributeError)) {
                 setNotFoundException(currentKey, currentVal);
-                if (i > 0) {
-                    Py_DECREF(currentVal);
-                }
-                return NULL;
             }
+            // any exceptions results in failure
+            if (i > 0) {
+                Py_DECREF(currentVal);
+            }
+            return NULL;
           }
         }
         if (i > 0) {