Discussion:
[[inferior events] partial cleanup for copy_py_list
Tom Tromey
2010-12-14 21:41:46 UTC
Permalink
This fixes some issues in copy_py_list, but not all.

The patch changes it to check PyList_Append.

I think it should probably use an iterator to go through the list.
Otherwise it is susceptible to a bug where another thread changes the
list while we are iterating over it.

Still, ok to push in this form?

Tom

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 7c686d3..34e5bf8 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -163,10 +163,11 @@ compute_python_string (struct command_line *l)
}

/* Returns a a copy of the give LIST.
- Creates a new reference which must be handeld by the caller. */
+ Creates a new reference which must be handled by the caller. */

PyObject *
-copy_py_list (PyObject *list){
+copy_py_list (PyObject *list)
+{
int i;

PyObject *new_list = PyList_New (0);
@@ -174,7 +175,11 @@ copy_py_list (PyObject *list){
return NULL;

for (i = 0; i < PyList_Size (list); i++)
- PyList_Append (new_list, PyList_GET_ITEM (list, i));
+ if (PyList_Append (new_list, PyList_GET_ITEM (list, i)) < 0)
+ {
+ Py_DECREF (new_list);
+ return NULL;
+ }

return new_list;
}
sami wagiaalla
2010-12-15 15:57:49 UTC
Permalink
Post by Tom Tromey
This fixes some issues in copy_py_list, but not all.
The patch changes it to check PyList_Append.
I think it should probably use an iterator to go through the list.
Otherwise it is susceptible to a bug where another thread changes the
list while we are iterating over it.
Hmm I did not know we had the option of using a safe iterator. If we do
then that eliminates the need for copy_py_list in the first place;
copy_py_list is used to make the call back of listeners safe against one
listener editing the list during the iteration by removing an element.
Post by Tom Tromey
Still, ok to push in this form?
Yes. And thanks for the cleanups.

Sami
Tom Tromey
2010-12-15 16:42:12 UTC
Permalink
Tom> I think it should probably use an iterator to go through the list.
Tom> Otherwise it is susceptible to a bug where another thread changes the
Tom> list while we are iterating over it.

Sami> Hmm I did not know we had the option of using a safe iterator. If we
Sami> do then that eliminates the need for copy_py_list in the first place;
Sami> copy_py_list is used to make the call back of listeners safe against
Sami> one listener editing the list during the iteration by removing an
Sami> element.

TBH, I am not totally sure how things work in this area.

One option would be to use the existing convenience function
PyList_AsTuple, and just trust the Python core to get it right.

Tom

Loading...