bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text and Element.tail #3924

orenmn · 2017-10-08T21:09:28Z

In addition, add tests to test_xml_etree_c to verify that the crashes are no more.

https://bugs.python.org/issue31728

serhiy-storchaka · 2017-10-09T21:54:29Z

Modules/_elementtree.c


    Py_INCREF(Py_None);
-    Py_DECREF(JOIN_OBJ(self->text));
+    _clear_joined_ptr(&self->text);


Setting self->text to NULL is not safe. In this case the "text" getter will return NULL without setting an exception.

serhiy-storchaka · 2017-10-09T21:55:35Z

Lib/test/test_xml_etree_c.py

+        elem = cET.Element('elem')
+        class X:
+            def __del__(self):
+                elem.clear()


Getting elem.text or elem.tail before calling elem.clear() will crash.

bedevere-bot · 2017-10-09T21:55:46Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…lf->tail to NULL

orenmn · 2017-10-10T08:55:26Z

In the end of element_init(), there are 8 lines that can be replaced by 2 calls to _set_joined_ptr(). I wasn't sure whether this would be an improvement or a code churn, so i didn't change it.

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-10-10T08:55:28Z

Nobody expects the Spanish Inquisition!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka

I think this would be an improvement if use the new helper in element_init(), _elementtree_Element___copy___impl(), _elementtree_Element___deepcopy___impl().

It may be worth to move Py_INCREF() out of _set_joined_ptr(). This will help in _elementtree_Element___deepcopy___impl() and will allow to implement _clear_joined_ptr(p) as _set_joined_ptr(p, NULL).

serhiy-storchaka · 2017-10-10T09:42:21Z

Modules/_elementtree.c

 }

+/* Py_INCREF followed by Py_SETREF for a PyObject* that uses a join flag. */
+static void _set_joined_ptr(PyObject **p, PyObject *new_joined_ptr)


Move static void on a separate line. Or better even use LOCAL(void) or Py_LOCAL_INLINE(void).

* implement _clear_joined_ptr() using _set_joined_ptr(). * use _set_joined_ptr() in more methods.

serhiy-storchaka · 2017-10-10T16:56:44Z

Modules/_elementtree.c

-    Py_DECREF(JOIN_OBJ(element->text));
-    element->text = self->text;
+    _set_joined_ptr(&element->text, self->text);
    Py_INCREF(JOIN_OBJ(element->text));


The new reference should be incremented before decrementing the old reference.

In this particular case the order doesn't matter since the old reference is None, and decrementing it can't trigger executing a user code. But for unifying the code I would move the incrementation above _set_joined_ptr().

serhiy-storchaka · 2017-10-10T17:02:18Z

Modules/_elementtree.c

-    _clear_joined_ptr(&self->text);
-    self->text = text ? JOIN_SET(text, PyList_CheckExact(text)) : Py_None;
-    Py_INCREF(JOIN_OBJ(self->text));
+    new_val = text ? JOIN_SET(text, PyList_CheckExact(text)) : Py_None;


You can reuse the text variable for the new value. The old value no longer used.

…ing variables.

serhiy-storchaka · 2017-10-10T18:49:33Z

And the last comment. The NEWS.d entry should be in the "Library" section.

miss-islington · 2017-10-10T20:26:28Z

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

miss-islington · 2017-10-10T20:26:29Z

@orenmn and @serhiy-storchaka, Something is wrong... I can't backport for now.
Please backport using cherry_picker on command line.
cherry_picker 39ecb9c71b6e55d8a61a710d0144231bd88f9ada 3.6

serhiy-storchaka · 2017-10-10T20:27:16Z

Thank you @orenmn! Do you mind to make backports?

miss-islington · 2017-10-10T20:27:35Z

@orenmn and @serhiy-storchaka, Something is wrong... I can't backport for now.
Please backport using cherry_picker on command line.
cherry_picker 39ecb9c71b6e55d8a61a710d0144231bd88f9ada 2.7

miss-islington · 2017-10-10T21:01:53Z

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2017-10-10T21:02:09Z

GH-3945 is a backport of this pull request to the 3.6 branch.

…p of Element.text and Element.tail (pythonGH-3924) (cherry picked from commit 39ecb9c)

miss-islington · 2017-10-10T21:02:58Z

Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 39ecb9c71b6e55d8a61a710d0144231bd88f9ada 2.7

…p of Element.text and Element.tail (GH-3924) (#3945) (cherry picked from commit 39ecb9c)

orenmn · 2017-10-11T08:48:35Z

(i am working on a backport to 2.7)

bedevere-bot · 2017-10-11T10:27:12Z

GH-3950 is a backport of this pull request to the 2.7 branch.

…p of Element.text and Element.tail (GH-3924) (#3950)

orenmn added 4 commits October 8, 2017 23:30

init commit

e6ab8f0

Merge branch 'master' into bpo31728-fix-crashes

e769b8f

dont use a single char variable name

28c6a85

add comments to clarify the test

3563f7e

the-knights-who-say-ni added the CLA signed label Oct 8, 2017

bedevere-bot added the awaiting review label Oct 8, 2017

serhiy-storchaka requested changes Oct 9, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 9, 2017

orenmn added 5 commits October 10, 2017 08:14

Merge branch 'master' into bpo31728-fix-crashes

abbb7dd

fix assertion failures which are a result of setting self->text or se…

4b5de02

…lf->tail to NULL

cosmetic change to a comment

2227e55

make sure to first incref the new value.

6604279

improve the comment of _set_joined_ptr and its 2nd argument name

d2416c4

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 10, 2017

serhiy-storchaka reviewed Oct 10, 2017

View reviewed changes

* use Py_LOCAL_INLINE().

bfb724c

* implement _clear_joined_ptr() using _set_joined_ptr(). * use _set_joined_ptr() in more methods.

serhiy-storchaka approved these changes Oct 10, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 10, 2017

serhiy-storchaka added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Oct 10, 2017

Make sure incref comes before decref, and simplify the C code by reus…

4e513cb

…ing variables.

serhiy-storchaka approved these changes Oct 10, 2017

View reviewed changes

move the NEWS.d item into Library

b05ddd3

serhiy-storchaka merged commit 39ecb9c into python:master Oct 10, 2017

bedevere-bot removed the awaiting merge label Oct 10, 2017

Mariatta added needs backport to 3.6 and removed needs backport to 3.6 labels Oct 10, 2017

bedevere-bot removed the needs backport to 3.6 label Oct 10, 2017

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 10, 2017

[3.6] bpo-31728: Prevent crashes in _elementtree due to unsafe cleanu…

ae896c6

…p of Element.text and Element.tail (pythonGH-3924) (cherry picked from commit 39ecb9c)

serhiy-storchaka pushed a commit that referenced this pull request Oct 10, 2017

[3.6] bpo-31728: Prevent crashes in _elementtree due to unsafe cleanu…

a8ac71d

…p of Element.text and Element.tail (GH-3924) (#3945) (cherry picked from commit 39ecb9c)

bedevere-bot removed the needs backport to 2.7 label Oct 11, 2017

serhiy-storchaka pushed a commit that referenced this pull request Oct 11, 2017

[2.7] bpo-31728: Prevent crashes in _elementtree due to unsafe cleanu…

f15058a

…p of Element.text and Element.tail (GH-3924) (#3950)

Uh oh!

bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text and Element.tail #3924

bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text and Element.tail #3924

Uh oh!

Conversation

orenmn commented Oct 8, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 9, 2017

Uh oh!

orenmn commented Oct 10, 2017

Uh oh!

bedevere-bot commented Oct 10, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Oct 10, 2017

Uh oh!

miss-islington commented Oct 10, 2017

Uh oh!

miss-islington commented Oct 10, 2017

Uh oh!

serhiy-storchaka commented Oct 10, 2017

Uh oh!

miss-islington commented Oct 10, 2017

Uh oh!

miss-islington commented Oct 10, 2017

Uh oh!

bedevere-bot commented Oct 10, 2017

Uh oh!

miss-islington commented Oct 10, 2017

Uh oh!

orenmn commented Oct 11, 2017

Uh oh!

bedevere-bot commented Oct 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Oct 8, 2017 •

edited by bedevere-bot

Loading