[Patches] PyObject_GetAttr/PyObject_SetAttr core dumps if given non-string names

M.-A. Lemburg mal@lemburg.com
Fri, 23 Jun 2000 10:04:48 +0200


Gisle Aas wrote:
> 
> "M.-A. Lemburg" <mal@lemburg.com> writes:
> 
> > > Index: Objects/object.c
> > > ===================================================================
> > > RCS file: /cvsroot/python/python/dist/src/Objects/object.c,v
> > > retrieving revision 2.71
> > > diff -u -p -u -r2.71 object.c
> > > --- Objects/object.c    2000/06/09 16:20:39     2.71
> > > +++ Objects/object.c    2000/06/22 19:27:51
> > > @@ -600,8 +600,12 @@ PyObject_GetAttr(v, name)
> > >  {
> > >         if (v->ob_type->tp_getattro != NULL)
> > >                 return (*v->ob_type->tp_getattro)(v, name);
> > > -       else
> > > +       else if (PyString_Check(name))
> > >                 return PyObject_GetAttrString(v, PyString_AsString(name));
> > > +       else {
> > > +               PyErr_SetString(PyExc_TypeError,
> > > +                               "attribute name must be string");
> > > +       }
> > >  }
> >
> > Hmm, I think it would be more generic to add a NULL
> > check to PyObject_GetAttrString() (PyString_AsString() returns
> > NULL in case name is not a string object).
> 
> Ok.  But it also set up the exception to be BadInternalCall() which
> does not feel right :-)

But that's what it is, right ? ;-)
 
> I also forgot to "return NULL" here.
> 
> > >  int
> > > @@ -626,12 +630,18 @@ PyObject_SetAttr(v, name, value)
> > >  {
> > >         int err;
> > >         Py_INCREF(name);
> > > -       PyString_InternInPlace(&name);
> > >         if (v->ob_type->tp_setattro != NULL)
> > >                 err = (*v->ob_type->tp_setattro)(v, name, value);
> > > -       else
> > > +       else if (PyString_Check(name)) {
> > > +               PyString_InternInPlace(&name);
> > >                 err = PyObject_SetAttrString(
> > >                         v, PyString_AsString(name), value);
> > > +       }
> > > +       else {
> > > +               PyErr_SetString(PyExc_TypeError,
> > > +                               "attribute name must be string");
> > > +               err = -1;
> > > +       }
> >
> > Here, I think you ought to move the string test just before
> > the interning call.
> 
> But you still want to be able to pass non-string objects to
> v->ob_type->tp_setattro I guess.  Anyway, an alternative patch is
> included here.
> 
> Thanks for the quick feedback!
> 
> Regards,
> Gisle
> 
> Index: Objects/object.c
> ===================================================================
> RCS file: /cvsroot/python/python/dist/src/Objects/object.c,v
> retrieving revision 2.71
> diff -u -p -u -r2.71 object.c
> --- Objects/object.c    2000/06/09 16:20:39     2.71
> +++ Objects/object.c    2000/06/22 20:37:21
> @@ -598,10 +598,17 @@ PyObject_GetAttr(v, name)
>         PyObject *v;
>         PyObject *name;
>  {
> +       char *name_str;
>         if (v->ob_type->tp_getattro != NULL)
>                 return (*v->ob_type->tp_getattro)(v, name);
> -       else
> -               return PyObject_GetAttrString(v, PyString_AsString(name));
> +
> +       name_str = PyString_AsString(name);
> +       if (name_str == NULL) {
> +               PyErr_SetString(PyExc_TypeError,
> +                               "attribute name must be string");
> +               return NULL;
> +       }
> +       return PyObject_GetAttrString(v, name_str);
>  }

This one looks ok. I would still be curious how the size
increase affects performance (you should note that the
above API is one of the more often used ones in Python).
I would guess that the addition of a variable makes inlining
harder -- so why not inline PyString_AsString() here:

if (!PyString_Check(name)) {
   ...error...
   return NULL;
}
return PyObject_GetAttrString(v, PyString_AS_STRING(name));

>  int
> @@ -625,13 +632,23 @@ PyObject_SetAttr(v, name, value)
>         PyObject *value;
>  {
>         int err;
> +       int name_is_string = 0;
>         Py_INCREF(name);
> -       PyString_InternInPlace(&name);
> +       if (PyString_Check(name)) {
> +               PyString_InternInPlace(&name);
> +               name_is_string = 1;
> +       }
>         if (v->ob_type->tp_setattro != NULL)
>                 err = (*v->ob_type->tp_setattro)(v, name, value);
> -       else
> +       else if (name_is_string) {

It's better to avoid this variable and repeat the PyString_Check()
(the check only tests a pointer, so it doesn't cost much more).

This would also allow using PyString_AS_STRING() instead
of the function _AsString().

>                 err = PyObject_SetAttrString(
>                         v, PyString_AsString(name), value);
> +       }
> +       else {
> +               PyErr_SetString(PyExc_TypeError,
> +                               "attribute name must be string");
> +               err = -1;
> +       }
>         Py_DECREF(name);
>         return err;
>  }
> 
>                        I confirm that, to the best of my knowledge and belief, this
>                        contribution is free of any claims of third parties under
>                        copyright, patent or other rights or interests ("claims").  To
>                        the extent that I have any such claims, I hereby grant to CNRI a
>                        nonexclusive, irrevocable, royalty-free, worldwide license to
>                        reproduce, distribute, perform and/or display publicly, prepare
>                        derivative versions, and otherwise use this contribution as part
>                        of the Python software and its related documentation, or any
>                        derivative versions thereof, at no cost to CNRI or its licensed
>                        users, and to authorize others to do so.
> 
>                        I acknowledge that CNRI may, at its sole discretion, decide
>                        whether or not to incorporate this contribution in the Python
>                        software and its related documentation.  I further grant CNRI
>                        permission to use my name and other identifying information
>                        provided to CNRI by me for use in connection with the Python
>                        software and its related documentation.

-- 
Marc-Andre Lemburg
______________________________________________________________________
Business:                                      http://www.lemburg.com/
Python Pages:                           http://www.lemburg.com/python/