bpo-31455: avoid calling "PyObject_GetAttrString()" with a live exception set by scoder · Pull Request #3545 · python/cpython

scoder · 2017-09-13T14:30:29Z

https://bugs.python.org/issue31455

…ser code) with a live exception set.

serhiy-storchaka · 2017-09-13T16:36:14Z

Please open an issue on the bug tracker. This isn't a trivial change.

I think that it would be better to silence only AttributeError.

scoder · 2017-09-13T17:30:48Z

Found as part of #3279, which started showing side-effects due to this bug.

…ser() and propagate all other exceptions.

scoder · 2017-09-13T17:54:24Z

PR updated, test and News entry added.

serhiy-storchaka

LGTM. I left just few nitpicks.

serhiy-storchaka · 2017-09-14T12:18:38Z

Lib/test/test_xml_etree.py

+
+        ET.XMLParser(target=RaisingBuilder(what=AttributeError))
+        for event in ('start', 'data', 'end', 'comment', 'pi'):
+            ET.XMLParser(target=RaisingBuilder(event, what=AttributeError))


Close the parser explicitly. Just for the case.

Done. (Requires parsing something, though.)

serhiy-storchaka · 2017-09-14T12:27:21Z

Lib/test/test_xml_etree.py

        self.assertEqual(child.tail, 'tail')
        self.assertEqual(child.attrib, {})

+    def test_exceptions(self):


Wouldn't XMLParserTest be better place? In any case I think it is better to place this method at the end of the class, after testing normal cases.

And maybe use more specific method name? "test_builder_attribute_errors" or something like.

Done. I kept it in the test class because the exceptions are related to the behaviour of the TreeBuilder.

serhiy-storchaka · 2017-09-14T12:35:52Z

Modules/_elementtree.c

 }

+static int
+ignore_attribute_error(PyObject *value) {


Put this { on new line.

serhiy-storchaka · 2017-09-14T12:37:20Z

Modules/_elementtree.c

    self->target = target;

    self->handle_start = PyObject_GetAttrString(target, "start");
+    if (ignore_attribute_error(self->handle_start))


New PEP 7 rules require braces even around simple if body.

Done. (Sad in this case, given the relation between "active" code and error handling code.)

…isbehaviour, not basic behaviour.

scoder · 2017-09-14T13:14:03Z

PR updated.

miss-islington · 2017-09-14T20:00:05Z

Thanks @scoder 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-09-14T20:00:10Z

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

…pythonGH-3545) * Avoid calling "PyObject_GetAttrString()" (and potentially executing user code) with a live exception set. * Ignore only AttributeError on attribute lookups in ElementTree.XMLParser() and propagate all other exceptions. (cherry picked from commit c8d8e15)

bedevere-bot · 2017-09-14T20:00:16Z

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

…GH-3545) (#3585) * Avoid calling "PyObject_GetAttrString()" (and potentially executing user code) with a live exception set. * Ignore only AttributeError on attribute lookups in ElementTree.XMLParser() and propagate all other exceptions. (cherry picked from commit c8d8e15)

miss-islington · 2017-09-27T05:51:56Z

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

miss-islington · 2017-09-27T05:51:57Z

Sorry, @scoder 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 c8d8e15bfc24abeeaaf3d8be9073276b0c011cdf 2.7

Avoid calling "PyObject_GetAttrString()" (and potentially executing u…

a9a6875

…ser code) with a live exception set.

the-knights-who-say-ni added the CLA signed label Sep 13, 2017

bedevere-bot added the awaiting review label Sep 13, 2017

scoder mentioned this pull request Sep 13, 2017

bpo-31336: Speed up type creation, which is highly dominated by slow dict lookups. #3279

Merged

scoder changed the title ~~Avoid calling "PyObject_GetAttrString()" with a live exception set.~~ bpo-31455: avoid calling "PyObject_GetAttrString()" with a live exception set Sep 13, 2017

Ignore only AttributeError on attribute lookups in ElementTree.XMLPar…

e0b8790

…ser() and propagate all other exceptions.

Add News entry for bpo-31544

5253d56

serhiy-storchaka reviewed Sep 14, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 14, 2017

Stefan Behnel added 2 commits September 14, 2017 15:10

Give test a more specific name and move it further down as it tests m…

3e1c77b

…isbehaviour, not basic behaviour.

Fix C code style.

8cb95c6

serhiy-storchaka approved these changes Sep 14, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 14, 2017

serhiy-storchaka closed this Sep 14, 2017

serhiy-storchaka reopened this Sep 14, 2017

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

serhiy-storchaka merged commit c8d8e15 into python:master Sep 14, 2017

bedevere-bot removed the needs backport to 3.6 label Sep 14, 2017

serhiy-storchaka added needs backport to 2.7 and removed needs backport to 2.7 labels Sep 27, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

scoder mentioned this pull request Oct 14, 2017

[2.7] bpo-31455: avoid calling "PyObject_GetAttrString()" with a live exception set #3992

Merged

serhiy-storchaka removed the needs backport to 2.7 label Mar 31, 2018

Uh oh!

Conversation

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

Uh oh!

Uh oh!

serhiy-storchaka commented Sep 13, 2017

Uh oh!

scoder commented Sep 13, 2017

Uh oh!

scoder commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scoder commented Sep 14, 2017

Uh oh!

miss-islington commented Sep 14, 2017

Uh oh!

miss-islington commented Sep 14, 2017

Uh oh!

bedevere-bot commented Sep 14, 2017

Uh oh!

miss-islington commented Sep 27, 2017

Uh oh!

miss-islington commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scoder commented Sep 13, 2017 •

edited by bedevere-bot

Loading

scoder commented Sep 13, 2017 •

edited

Loading