Skip to content

gh-111784: Add PyCapsule_ImportCapsule and fix segfault in _elementtree.c#112053

Closed
Eclips4 wants to merge 1 commit into
python:mainfrom
Eclips4:issue-111784
Closed

gh-111784: Add PyCapsule_ImportCapsule and fix segfault in _elementtree.c#112053
Eclips4 wants to merge 1 commit into
python:mainfrom
Eclips4:issue-111784

Conversation

@Eclips4

@Eclips4 Eclips4 commented Nov 14, 2023

Copy link
Copy Markdown
Member

That's a PoC.

We can add a NEWS entry and doc about new public API later, if we consider this as the right solution.

Also, I was incorrect in issue. We need to keep reference to the capsule, not pyexpat module.

@Eclips4

Eclips4 commented Nov 14, 2023

Copy link
Copy Markdown
Member Author

@mgorny can you confirm that this fixes the issue?

@Eclips4
Eclips4 marked this pull request as draft November 14, 2023 10:55
Comment thread Objects/capsule.c
"PyCapsule_Import \"%s\" is not valid",
name);
}
EXIT:

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.

We don't need here Py_XDECREF(object) as in the PyCapsule_Import, because we do it later manually. So, we can control a process of capsule's deallocation

@mgorny

mgorny commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

@mgorny can you confirm that this fixes the issue?

I've applied it on top of 3.12.0 and I still see a segv with my reproducer from #111784.

@Eclips4

Eclips4 commented Nov 14, 2023

Copy link
Copy Markdown
Member Author

@mgorny can you confirm that this fixes the issue?

I've applied it on top of 3.12.0 and I still see a segv with my reproducer from #111784.

Indeed. Interesting, reduced reproducer by @chgnrdv doesn't segfault'ed anymore, but this doesn't relevant for your example :(

@chgnrdv

chgnrdv commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

@Eclips4, it is reduced but not minimal :)
pyexpat/_elementtree has nothing to do with asyncio, and I guess that instantiation of _asyncio.Task just interferes with GC state (that may be invalid after _elementtree import) is some weakly deterministic way, which triggers segfault. We can't use this example as a stable way to reproduce the issue, and it could easily stop to work after some _asyncio changes being merged into main branch, for example.

@chgnrdv

chgnrdv commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

Other, a bit hacky repro:

import _elementtree
x = _elementtree.XMLParser()
import sys
del sys.modules['pyexpat']

Looks obvious in hindsight, as all what we need is to deallocate pyexpat capsule earlier than XMLParser instance.

Comment thread Objects/capsule.c
name);
}
EXIT:
if (name_dup) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

name_dup is always non-NULL here, as we return MemoryError at line 236 if PyMem_Malloc fails.

@Eclips4 Eclips4 Nov 14, 2023

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.

It's not a final version 😅
Currently we just talking about idea, sadly, but this solution is incorrect :(
FYI, it's just copied from PyCapsule_Import

@Eclips4

Eclips4 commented Nov 14, 2023

Copy link
Copy Markdown
Member Author

Other, a bit hacky repro:

import _elementtree
x = _elementtree.XMLParser()
import sys
del sys.modules['pyexpat']

Looks obvious in hindsight, as all what we need is to deallocate pyexpat capsule earlier than XMLParser instance.

Yep, I thinking about the same.
However.. this code doesn't segfault's on this patch..

@Eclips4

Eclips4 commented Dec 2, 2023

Copy link
Copy Markdown
Member Author

This PR fixes only one segfault (there's two segfaults actually) and way to fix this segfault is really weird. So I'm closing this.

@Eclips4 Eclips4 closed this Dec 2, 2023
@Eclips4
Eclips4 deleted the issue-111784 branch December 3, 2023 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants