PySequence_SetItem
Bill Pursell
The following code is pretty much straight out of
section 1.2.1.1 of the Python/C reference manual:
#include <Python.h>
int
main(void)
{
PyObject *l, *x;
Py_Initialize();
l = PyList_New(3);
x = PyInt_FromLong(1L);
if (l == NULL || x == NULL) {
PyErr_Print();
exit (EXIT_FAILURE);
}
PySequence_SetItem(l, 0, x);
Py_DECREF(x);
Py_Finalize();
return EXIT_SUCCESS;
}
Unforunately, it doesn't work. It segfaults, and
the error occurs in list_ass_item() when the
Py_DECREF macro is applied to old_value, which
was set at line 699 of Objects/listobject.c
with old_value = a->ob_item[i]; (old_value is
being set to NULL, and Py_DECREF is
being applied to NULL...boom!)
I'm totally new to embedding/extending python,
so I'm not sure if I'm doing something incredibly
stupid, but it certainly looks like PySequence_SetItem()
was expecting that there should already be an
item at the desired index.
Am I doing something stupid?
--
Bill Pursell
Bill Pursell
Bill Pursell wrote:
> The following code is pretty much straight out of
> section 1.2.1.1 of the Python/C reference manual:
>
> #include <Python.h>
>
> int
> main(void)
> {
> PyObject *l, *x;
>
> Py_Initialize();
>
> l = PyList_New(3);
> x = PyInt_FromLong(1L);
>
> if (l == NULL || x == NULL) {
> PyErr_Print();
> exit (EXIT_FAILURE);
> }
> PySequence_SetItem(l, 0, x);
> Py_DECREF(x);
>
> Py_Finalize();
> return EXIT_SUCCESS;
> }
Also note that the problem goes away if I replace
the call to PySequence_SetItem with:
PySequence_SetItem(l, 0, PyInt_FromLong(1L));
but the problem persists if I add a Py_INCREF(x)
after the assignment of x. (Which seems like a
completely silly thing to do, but I'm grasping
at strings...)
John Machin
> Bill Pursell wrote:
>
> Also note that the problem goes away if I replace
> the call to PySequence_SetItem with:
> PySequence_SetItem(l, 0, PyInt_FromLong(1L));
Are you sure? It should make absolutely no difference.
My experience so far:
1. crashes in call to PySequence_SetItem
2. removed py_DECREF, s/Sequence/List/ -> works OK
3. added code to test return value from PyWhatever_SetItem [you should
*always* test for error], s/Sequence/Object/ -> PyObject_SetItem call
returns -1, PyErr_Print -> "SystemError: null argument to internal
routine"
Looks like a bug or two. I'll rummage a bit more.
Cheers,
John
Jack Diederich
Changing the PySequence_SetItem to PyList_SetItem and dropping the
DECREF works for me too (PyList functions steal a reference). I also
tried setting the list to length 1, still no dice. The PySequence
version segs under 2.4 and 2.5. It segs even when the Int is changed
to a String.
Yikes, I'll poke around some more.
-Jack
John Machin
Jack Diederich wrote:
> Changing the PySequence_SetItem to PyList_SetItem and dropping the
> DECREF works for me too (PyList functions steal a reference). I also
> tried setting the list to length 1, still no dice. The PySequence
> version segs under 2.4 and 2.5. It segs even when the Int is changed
> to a String.
>
> Yikes, I'll poke around some more.
Yikes indeed.
Not the OP's problem, but a bug in the manual: example in the chapter
that the OP was reading acts as though the 2nd arg to PyObject_SetItem
is a C int (as it is for the List and Sequence varieties) -- it is in
fact a (PyObject *), which explains the SystemError that I got.
Cheers,
John
Jack Diederich
The good news is that this is a documentation problem.
When working at the C level you use the concrete API when you can
(PyList_*) and at the abstract level (PySequence_*) only when someone
passes you something and you don't know what it is, you only know it
supports the Sequence API.
The exmample should use the PyList API to construct the list. The
Sequence API requires valid objects to work. Here the object is only half
constructed and list_ass_item tries to DECREF the zeroth member which
is NULL because the list isn't constructed yet. PyList_SetItem does
an XDECREF (decrement only if the pointer isn't null) because it expects
to be used that way.
-Jack
John Machin
OK, here's the story:
PyList_New fills the list with NULLs.
PyList_SetItem applies Py_XDECREF to the previous contents.
PySequence_SetItem calls the function in the object's sq_ass_item slot.
This is list_ass_item, which as the OP noted, applies Py_DECREF to the
previous contents. Splat when NULL.
I wonder how long it's been like that?
FYI, the buggy manual section is
http://docs.python.org/api/refcountDetails.html
Cheers,
John
John Machin
Jack Diederich wrote:
> On Wed, Aug 16, 2006 at 02:39:24PM -0700, John Machin wrote:
> >
> > Jack Diederich wrote:
> >
> > > Changing the PySequence_SetItem to PyList_SetItem and dropping the
> > > DECREF works for me too (PyList functions steal a reference). I also
> > > tried setting the list to length 1, still no dice. The PySequence
> > > version segs under 2.4 and 2.5. It segs even when the Int is changed
> > > to a String.
> > >
> > > Yikes, I'll poke around some more.
> >
> > Yikes indeed.
> >
> > Not the OP's problem, but a bug in the manual: example in the chapter
> > that the OP was reading acts as though the 2nd arg to PyObject_SetItem
> > is a C int (as it is for the List and Sequence varieties) -- it is in
> > fact a (PyObject *), which explains the SystemError that I got.
>
> The good news is that this is a documentation problem.
Cop-out alert! I disagree. Elsewhere Python goes to great lengths to
check for nasties instead of just segfaulting.
> When working at the C level you use the concrete API when you can
> (PyList_*) and at the abstract level (PySequence_*) only when someone
> passes you something and you don't know what it is, you only know it
> supports the Sequence API.
>
> The exmample should use the PyList API to construct the list. The
> Sequence API requires valid objects to work. Here the object is only half
> constructed and list_ass_item tries to DECREF the zeroth member which
> is NULL because the list isn't constructed yet. PyList_SetItem does
> an XDECREF (decrement only if the pointer isn't null) because it expects
> to be used that way.
PyList_SetItem is not sentient. It expects nothing. Now tell us what
the developer was thinking -- certainly not consistency with the
documantation. If the Sequence API requires valid objects to work,
then in this case it should raise an error.
In fact that manual section calls using the PySequence_SetItem the
recommended approach!!!
Are you suggesting a rework of the manual instead of inserting a X in
the offending py_DECREF?
Sheesh all round ...
Jack Diederich
It takes a big man to admit when he's wrong so I'll blame low blood sugar.
Since this had been the same going all the way back to 2.2 I assumed it was
common. It isn't, simlar functions in other objects guard against the
possibility.
I'll check in the change assuming no one else has already (and assuming
the trunk isn't currently frozen, I'll have to check).
-Jack
John Machin
Jack Diederich wrote:
> On Wed, Aug 16, 2006 at 03:25:39PM -0700, John Machin wrote:
> > > >
> > > > Not the OP's problem, but a bug in the manual: example in the chapter
> > > > that the OP was reading acts as though the 2nd arg to PyObject_SetItem
> > > > is a C int (as it is for the List and Sequence varieties) -- it is in
> > > > fact a (PyObject *), which explains the SystemError that I got.
Please don't forget this which is definitely a bug in the docs.
> > >
> > > The good news is that this is a documentation problem.
> >
> > Cop-out alert! I disagree. Elsewhere Python goes to great lengths to
> > check for nasties instead of just segfaulting.
> >
> > > When working at the C level you use the concrete API when you can
> > > (PyList_*) and at the abstract level (PySequence_*) only when someone
> > > passes you something and you don't know what it is, you only know it
> > > supports the Sequence API.
> > >
> > > The exmample should use the PyList API to construct the list. The
> > > Sequence API requires valid objects to work. Here the object is only half
> > > constructed and list_ass_item tries to DECREF the zeroth member which
> > > is NULL because the list isn't constructed yet. PyList_SetItem does
> > > an XDECREF (decrement only if the pointer isn't null) because it expects
> > > to be used that way.
> >
> > PyList_SetItem is not sentient. It expects nothing. Now tell us what
> > the developer was thinking -- certainly not consistency with the
> > documantation. If the Sequence API requires valid objects to work,
> > then in this case it should raise an error.
> >
> > In fact that manual section calls using the PySequence_SetItem the
> > recommended approach!!!
> >
> > Are you suggesting a rework of the manual instead of inserting a X in
> > the offending py_DECREF?
> >
> > Sheesh all round ...
>
> It takes a big man to admit when he's wrong so I'll blame low blood sugar.
:-)
> Since this had been the same going all the way back to 2.2 I assumed it was
> common. It isn't, simlar functions in other objects guard against the
> possibility.
I'm a bit curious here: tuples naturally don't fill in the sq_ass_item
slot and I couldn't think of another object that would be initialised
with NULL(s) in the expectation of being filled in later or maybe not
... enlightenment, please.
>
> I'll check in the change assuming no one else has already (and assuming
> the trunk isn't currently frozen, I'll have to check).
For avoidance of doubt: the change is to use Py_XDECREF, yes/no?
Cheers,
John
Jack Diederich
>
> Jack Diederich wrote:
> > On Wed, Aug 16, 2006 at 03:25:39PM -0700, John Machin wrote:
>
> > > > >
> > > > > Not the OP's problem, but a bug in the manual: example in the chapter
> > > > > that the OP was reading acts as though the 2nd arg to PyObject_SetItem
> > > > > is a C int (as it is for the List and Sequence varieties) -- it is in
> > > > > fact a (PyObject *), which explains the SystemError that I got.
>
> Please don't forget this which is definitely a bug in the docs.
>
You should file a bug (or patch) against the docs. I haven't ever
updated them myself so I won't be the guy that ends up fixing it.
<snip>
> > Since this had been the same going all the way back to 2.2 I assumed it was
> > common. It isn't, simlar functions in other objects guard against the
> > possibility.
>
> I'm a bit curious here: tuples naturally don't fill in the sq_ass_item
> slot and I couldn't think of another object that would be initialised
> with NULL(s) in the expectation of being filled in later or maybe not
> ... enlightenment, please.
>
> > I'll check in the change assuming no one else has already (and assuming
> > the trunk isn't currently frozen, I'll have to check).
Nothing else is quite the same. As you point out almost nothing in core even
supports the sq_ass_item slot. Those that do aren't initialized with a fixed
size (deque takes an iterator) so they can't have NULLs in them. Tuples have
no ass_item so you _have to_ use the concrete API. Mappings which have their
own ass slot (like dicts) are sparse so they are used to handling nulls.
None of them allow you to segfault the interpreter.
The generic Sequence API isn't useful when you are initializing a sequence
object because it is either a list, a tuple, or some extension of your own
creation. Because you know what you are making it is easiest to use the
concrete API. Heck, if it is a tuple you have to. No one noticed the
bug because no one uses that API for initializing objects in real life.
It is handy for functions that take a mutable list as an argument.
> For avoidance of doubt: the change is to use Py_XDECREF, yes/no?
Yes.
-Jack
Fredrik Lundh
> Are you suggesting a rework of the manual instead of inserting a X in
> the offending py_DECREF?
are you suggesting slowing things down just because of a bug in the
documentation ? you cannot return an uninitialized list object to
Python anyway, so there's no need to add extra checks to a function
that's *documented* to be the equivalent of a *Python-level* sequence
assignment.
the "I spent time debugging this, so now everyone should suffer"
approach doesn't strike me as very Pythonic.
</F>
Fredrik Lundh
> It is handy for functions that take a mutable list as an argument.
an *existing*, and properly *initialized*, mutable sequence. PyList_New
doesn't give you such an object.
</F>
Fredrik Lundh
>> For avoidance of doubt: the change is to use Py_XDECREF, yes/no?
>
> Yes.
not necessarily: the bug is that you're using an uninitialized object in
a context that expects an initialized object. might be a better idea to
raise a SystemError exception.
</F>
John Machin
Fredrik Lundh wrote:
> John Machin wrote:
>
> > Are you suggesting a rework of the manual instead of inserting a X in
> > the offending py_DECREF?
>
> are you suggesting slowing things down just because of a bug in the
> documentation ?
Not explicitly; not intentionally.
> you cannot return an uninitialized list object to
> Python anyway, so there's no need to add extra checks to a function
> that's *documented* to be the equivalent of a *Python-level* sequence
> assignment.
1. It's also documented as being the recommended way of filling up a
list after PyList_New.
2. So you'd rather not change the code, and just let it segfault if
someone calls it (or PyObject_SetItem) instead of PyList_SetItem?
>
> the "I spent time debugging this, so now everyone should suffer"
> approach doesn't strike me as very Pythonic.
That's the wildest leap from scant evidence to a wrong conclusion that
I've seen for quite a while :-)
Anyway, having had a look at the code:
#define Py_XDECREF(op) if ((op) == NULL) ; else Py_DECREF(op)
I have to say that I'm truly sorry, I wasn't aware that that little
"skip if zero" chunk of code was up there with cancer, AIDS and bombs
-- I'll pull my head in and not refer to the topic again.
Fredrik Lundh
> 1. It's also documented as being the recommended way of filling up a
> list after PyList_New.
since it doesn't work in any existing Python release, it's hardly "recommended".
the Python documentation has never been formally binding; if the documentation
doesn't match the code, it's usually the documentation that's flawed.
> 2. So you'd rather not change the code, and just let it segfault if
> someone calls it (or PyObject_SetItem) instead of PyList_SetItem?
as I said, you're using an API that's designed for use on *properly initialized*
objects on an object that *hasn't been initialized*. if you're going to patch all
places where that can happen, you might as well rewrite the entire interpreter.
the documentation is broken, and *must* be fixed. catching this specific case
in the code is a lot less important; it's just one of many possible errors you can
make when writing C-level code. there's simply no way you can catch them
all.
</F>
Jack Diederich
> John Machin wrote:
>
> > 1. It's also documented as being the recommended way of filling up a
> > list after PyList_New.
>
>
> the Python documentation has never been formally binding; if the documentation
> doesn't match the code, it's usually the documentation that's flawed.
>
> > someone calls it (or PyObject_SetItem) instead of PyList_SetItem?
>
> objects on an object that *hasn't been initialized*. if you're going to patch all
> places where that can happen, you might as well rewrite the entire interpreter.
>
> the documentation is broken, and *must* be fixed. catching this specific case
> in the code is a lot less important; it's just one of many possible errors you can
> make when writing C-level code. there's simply no way you can catch them
> all.
>
describe a bad practice - no one ever uses an abstract API to initialize the
concrete type they just created. For bonus points the example would have always
segfaulted (at least back to 2.2, I didn't check 1.5).
While it feels uneven that other types can't get this kind of segfault the
fact that no one else has ever run accross it makes the point moot.
Unrelated, it looks like PySequence_* doesn't have much reason to live now
that iterators are everywhere. In the core it mainly used to do the equiv of
def listize(ob):
if (type(ob) in (list, tuple)):
return ob
else:
return list(iter(ob))
-Jack