gh-146056: Fix TreeBuilder stack in xml.etree - #146062
Conversation
Sorry, something went wrong.
No longer create a stack of 20 items, but create an empty stack instead. It prevents crashes when the stack list is discovered by gc.get_referrers() or other functions. Fix also reference counting in treebuilder_handle_end().
|
I added "skip news" since this issue requires calling gc.get_referrers() which is used in "regular code". |
Sorry, something went wrong.
| item = self->last; | ||
| self->last = Py_NewRef(self->this); | ||
| Py_XSETREF(self->last_for_tail, self->last); | ||
| Py_XSETREF(self->last_for_tail, Py_NewRef(self->last)); |
There was a problem hiding this comment.
This is unrelated.
If rewrite this code to be more explicit or safe, I would write something like
PyObject *last = self->last;
PyObject *last_for_tail = self->last_for_tail;
PyObject *this = self->this;
self->index--;
self->this = Py_NewRef(PyList_GET_ITEM(self->stack, self->index));
self->last = Py_NewRef(this);
self->last_for_tail = Py_NewRef(this);
Py_DECREF(last);
Py_XDECREF(last_for_tail);
if (treebuilder_append_event(self, self->end_event_obj, this) < 0) {
Py_DECREF(this);
return NULL;
}
return this;But we should also look a the other ends -- how these attributes are set in other code in this file. This is a separate issue.
Sorry, something went wrong.
There was a problem hiding this comment.
I wrote #146167 which uses your suggestion.
Sorry, something went wrong.
| t->comment_factory = NULL; | ||
| t->pi_factory = NULL; | ||
| t->stack = PyList_New(20); | ||
| t->stack = PyList_New(0); |
There was a problem hiding this comment.
This can have performance impact.
Sorry, something went wrong.
|
I abandon my change since a more generic change was merged: #146129. |
Sorry, something went wrong.
No longer create a stack of 20 items, but create an empty stack instead. It prevents crashes when the stack list is discovered by gc.get_referrers() or other functions.
Fix also reference counting in treebuilder_handle_end().