Skip to content

gh-111784: Fix two segfaults #113405

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 24, 2023
Merged

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Dec 22, 2023

This PR contains two fixes.
First fix resolve situation when pyexpat module (which contains expat_CAPI capsule) deallocates before _elementtree, so we need to hold a strong reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before deallocation of XMLParser instances, which uses module state to clear some stuff.

(I'll write a NEWS entry later, if it is necessary)

@Eclips4
Copy link
Member Author

Eclips4 commented Dec 22, 2023

@mgorny Can you confirm that problem is resolved now?

@Eclips4
Copy link
Member Author

Eclips4 commented Dec 22, 2023

Also, I check it for reference leaks:

./python.exe -m test -R 3:3 test_xml_etree_c
Using random seed: 1167552768
0:00:00 load avg: 2.77 Run 1 test sequentially
0:00:00 load avg: 2.77 [1/1] test_xml_etree_c
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 2.7 sec
Total tests: run=208 skipped=6
Total test files: run=1/1
Result: SUCCESS

@mgorny
Copy link
Contributor

mgorny commented Dec 22, 2023

Thanks! I can confirm that with this patch, slixmpp's test suite passes without segfaults on top of Python 3.12.

@Eclips4 Eclips4 added the needs backport to 3.12 only security fixes label Dec 23, 2023
@serhiy-storchaka serhiy-storchaka self-requested a review December 23, 2023 10:23
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Yes, add a NEWS entry, please.

@Eclips4
Copy link
Member Author

Eclips4 commented Dec 23, 2023

LGTM.

Yes, add a NEWS entry, please.

Done.

@serhiy-storchaka serhiy-storchaka merged commit 894f0e5 into python:main Dec 24, 2023
@miss-islington-app
Copy link

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

@bedevere-app
Copy link

bedevere-app bot commented Dec 24, 2023

GH-113446 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 24, 2023
…H-113405)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
(cherry picked from commit 894f0e5)

Co-authored-by: Kirill Podoprigora <[email protected]>
@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 24, 2023
serhiy-storchaka pushed a commit that referenced this pull request Dec 24, 2023
) (GH-113446)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
(cherry picked from commit 894f0e5)

Co-authored-by: Kirill Podoprigora <[email protected]>
@Eclips4
Copy link
Member Author

Eclips4 commented Dec 24, 2023

Thanks Serhiy for your review!

@Eclips4 Eclips4 deleted the issue-111784 branch December 24, 2023 09:25
ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023
…H-113405)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
…H-113405)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…H-113405)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…H-113405)

First fix resolve situation when pyexpat module (which contains expat_CAPI
capsule) deallocates before _elementtree, so we need to hold a strong
reference to pyexpat module to.

Second fix resolve situation when module state is deallocated before
deallocation of XMLParser instances, which uses module state to clear
some stuff.
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.

3 participants