Skip to content

bpo-31499, xml.etree: Fix xmlparser_gc_clear() crash#3641

Merged
vstinner merged 5 commits into
python:masterfrom
vstinner:etree_gc_clear
Sep 18, 2017
Merged

bpo-31499, xml.etree: Fix xmlparser_gc_clear() crash#3641
vstinner merged 5 commits into
python:masterfrom
vstinner:etree_gc_clear

Conversation

@vstinner

@vstinner vstinner commented Sep 18, 2017

Copy link
Copy Markdown
Member

xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a
crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously
by the garbage collector, because the parser was part of a reference cycle.

https://bugs.python.org/issue31499

xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a
crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously
by the garbage collector, because the parser was part of a reference cycle.
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
Comment thread Modules/_elementtree.c Outdated
{
EXPAT(ParserFree)(self->parser);
if (self->parser != NULL) {
EXPAT(ParserFree)(self->parser);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be safer to set self->parser = NULL before calling ParserFree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it's overkill. EXPAT(ParserFree) calls a C function which is outside the "Python" world, I don't see how it could trigger a reentrant call.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I made the change anyway. It doesn't harm much :-)

@@ -0,0 +1,3 @@
xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much implementation details. They are good on the tracker or in commit message, but not in a changelog. Look at this from the point of view of common Python user.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I simplified the NEWS entry.

@vstinner

Copy link
Copy Markdown
Member Author

Thanks for your review @serhiy-storchaka!

@vstinner vstinner merged commit e727d41 into python:master Sep 18, 2017
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @Haypo for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@vstinner vstinner deleted the etree_gc_clear branch September 18, 2017 12:29
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 18, 2017
…3641)

* bpo-31499, xml.etree: Fix xmlparser_gc_clear() crash

xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a
crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously
by the garbage collector, because the parser was part of a reference cycle.

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
(cherry picked from commit e727d41)
@bedevere-bot

Copy link
Copy Markdown

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

vstinner pushed a commit that referenced this pull request Sep 18, 2017
…3645)

* bpo-31499, xml.etree: Fix xmlparser_gc_clear() crash

xml.etree: xmlparser_gc_clear() now sets self.parser to NULL to prevent a
crash in xmlparser_dealloc() if xmlparser_gc_clear() was called previously
by the garbage collector, because the parser was part of a reference cycle.

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
(cherry picked from commit e727d41)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants