Re: Segfault in name mapper

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.


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

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

iEYEARECAAYFAkyQPkwACgkQFCbH3D9R4W9u3ACeOq0aNetlOsjzxw/jOto4c8fa
L/8An0scJUS759Z5h9sxiD+F6nasy8LH
=ti7T
-----END PGP SIGNATURE-----