Re: Segfault in name mapper

  On 15/09/10 04:32, R. Tyler Croy wrote:
> On Mon, 13 Sep 2010, Jon Siddle wrote:
>
>>    It is possible to trivially cause the name mapper to segfault if
>> getattr throws an exception:
>
> I just sat down to try to apply this patch and I realized it looks like you
> passed along the output from git-diff(1). Would you mind using
> git-format-patch(1) to generate a commit patch? This will allow your commit
> message and attribution to be properly carried along.

Oops. I'm fairly new to git I'm afraid. See attached.
>
>> $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
>> 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) {
>>
>> ------------------------------------------------------------------------------
>> 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
> - R. Tyler Croy
> --------------------------------------
>    GitHub: http://github.com/rtyler
>   Twitter: http://twitter.com/agentdero
>


-- 
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

>From 19da0cb29f143a1559a1776dab7f6a01aef863ab Mon Sep 17 00:00:00 2001
From: Jon Siddle <[email protected]>
Date: Tue, 14 Sep 2010 15:08:26 +0100
Subject: [PATCH] Added test and fix for segfault in namemapper where exception is thrown
 by __getattr__.

---
 buildandrun                 |    3 ++-
 cheetah/Tests/NameMapper.py |   13 +++++++++++++
 cheetah/c/_namemapper.c     |   11 ++++++-----
 3 files changed, 21 insertions(+), 6 deletions(-)

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) {
-- 
1.6.6.1