Re: Segfault in name mapper

On Wed, 15 Sep 2010, Jon Siddle wrote:

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

Mahhhrrvelous. I've applied your commit and pushed it up to GitHub:
    http://github.com/rtyler/cheetah/commit/0287a0833cc30e0e469f6a774770002128a0281f

I'll do some more rigorous testing and cut a new release this evening when I
get back from $DAYJOB.


Once again, great catch and thanks for the patch.


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

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

- R. Tyler Croy
--------------------------------------
  GitHub: http://github.com/rtyler
 Twitter: http://twitter.com/agentdero

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)

iEYEARECAAYFAkyRCEoACgkQFCbH3D9R4W8r8gCePYwFsgdeGMDXaTeo6wPn12DO
nHgAn0GwIeXsX4cbxA3HzYt8EAfp+e7h
=8xxw
-----END PGP SIGNATURE-----