Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-111784: AddPyCapsule_ImportCapsule and fix segfault in_elementtree.c#112053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Eclips4 wants to merge1 commit intopython:mainfromEclips4:issue-111784

Conversation

@Eclips4
Copy link
Member

@Eclips4Eclips4 commentedNov 14, 2023•
edited
Loading

That's a PoC.

We can add aNEWS 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, notpyexpat module.

@Eclips4
Copy link
MemberAuthor

@mgorny can you confirm that this fixes the issue?

@Eclips4Eclips4 marked this pull request as draftNovember 14, 2023 10:55
"PyCapsule_Import \"%s\" is not valid",
name);
}
EXIT:
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

@mgorny
Copy link
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
Copy link
MemberAuthor

@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
Copy link
Contributor

chgnrdv commentedNov 14, 2023•
edited
Loading

@Eclips4, it is reduced but not minimal :)
pyexpat/_elementtree has nothing to do withasyncio, 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.

Eclips4 reacted with thumbs up emoji

@chgnrdv
Copy link
Contributor

Other, a bit hacky repro:

import_elementtreex=_elementtree.XMLParser()importsysdelsys.modules['pyexpat']

Looks obvious in hindsight, as all what we need is to deallocatepyexpat capsule earlier thanXMLParser instance.

name);
}
EXIT:
if (name_dup) {
Copy link
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 returnMemoryError at line 236 ifPyMem_Malloc fails.

Copy link
MemberAuthor

@Eclips4Eclips4Nov 14, 2023•
edited
Loading

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 fromPyCapsule_Import

@Eclips4
Copy link
MemberAuthor

Eclips4 commentedNov 14, 2023•
edited
Loading

Other, a bit hacky repro:

import_elementtreex=_elementtree.XMLParser()importsysdelsys.modules['pyexpat']

Looks obvious in hindsight, as all what we need is to deallocatepyexpat capsule earlier thanXMLParser instance.

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

@Eclips4
Copy link
MemberAuthor

Eclips4 commentedDec 2, 2023•
edited
Loading

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.

@Eclips4Eclips4 closed thisDec 2, 2023
@Eclips4Eclips4 deleted the issue-111784 branchDecember 3, 2023 07:05
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnerAwaiting requested review from vstinner

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

1 more reviewer

@chgnrdvchgnrdvchgnrdv left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Eclips4@mgorny@chgnrdv
[8]ページ先頭
©2009-2025 Movatter.jp