Skip to content

gh-100795: avoid unexpected freeaddrinfo after failed getaddrinfo #101220

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 22, 2023
Merged

gh-100795: avoid unexpected freeaddrinfo after failed getaddrinfo #101220

merged 4 commits into from
Jan 22, 2023

Conversation

sebres
Copy link
Contributor

@sebres sebres commented Jan 21, 2023

This is rebased #101010 to main.

Proposed PR fixes segfault gh-100795 - avoid unexpected freeaddrinfo if res becomes not NULL during invocation of getaddrinfo if it fails.
Previously this could cause double freeing and other hardly reproducible aftereffects, especially in multithreaded environment.

One could surely do return -1 instead of res = NULL & goto fail (in second case), like in

cpython/Modules/socketmodule.c

Line 1095 in 5ef90ee

return -1;

but this way it is minimal invasive (more consistent, remains safe against some merges or future implementations expecting goto fail to free some other handles, etc).

sebres added 2 commits January 21, 2023 12:38
@sebres
Update socketmodule.c
8886e75 
fixes segfault pythongh-100795 - avoid unexpected `freeaddrinfo` if `res` becomes not NULL during invocation of `getaddrinfo` if it fails
@sebres
typo in comment
4d56b42
@bedevere-bot bedevere-bot mentioned this pull request Jan 21, 2023
Closed
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@arhadthedev
Copy link
Member

I'll propose the news entry as a PR in your repo after https://github.com/sebres/cpython/tree/fix-gh-100795 gets cloned to my computer so I could branch off it.

@sebres sebres mentioned this pull request Jan 21, 2023
Closed
@sebres
Copy link
Contributor Author

sebres commented Jan 21, 2023

Thx!

I'll propose the news entry as a PR in your repo

Why? Can't you push directly to my branch?

git push https://github.com/sebres/cpython.git your-branch:fix-gh-100795

(since the branch is obviously not protected and it is under PR, GH'd allow to write there, at least python's members should have write rights).

But do what you consider to be right. Thanks again.

@arhadthedev arhadthedev mentioned this pull request Jan 21, 2023
Merged
@arhadthedev
Copy link
Member

Can't you push directly to my branch?

Only members of https://github.com/python can push to branches bound to PRs in https://github.com/python/cpython. I'm not a member, just a regular contributor.

@gpshead gpshead added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-crash A hard crash of the interpreter, possibly with a core dump labels Jan 22, 2023
@kumaraditya303 kumaraditya303 merged commit 5f08fe4 into python:main Jan 22, 2023
@miss-islington
Copy link
Contributor

Thanks @sebres for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

gpshead
gpshead reviewed Jan 22, 2023
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

I'm unsure that this PR is good, see my comment on the issue.

@gpshead gpshead removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jan 22, 2023
@bedevere-bot
Copy link

GH-101236 is a backport of this pull request to the 3.11 branch.

@bedevere-bot
Copy link

GH-101237 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 22, 2023
@sebres @arhadthedev @miss-islington
pythongh-100795: avoid unexpected freeaddrinfo after failed `getadd…
bada9c4 
…rinfo` (pythonGH-101220)

(cherry picked from commit 5f08fe4)

Co-authored-by: Sergey G. Brester <[email protected]>
Co-authored-by: Oleg Iarygin <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 22, 2023
@sebres @arhadthedev @miss-islington
pythongh-100795: avoid unexpected freeaddrinfo after failed `getadd…
bd8ed16 
…rinfo` (pythonGH-101220)

(cherry picked from commit 5f08fe4)

Co-authored-by: Sergey G. Brester <[email protected]>
Co-authored-by: Oleg Iarygin <[email protected]>
@kumaraditya303
Copy link
Contributor

I'm unsure that this PR is good, see my comment on the issue.

Oh sorry, I just merged it.

@kumaraditya303
Copy link
Contributor

I'll revert this, sorry @gpshead.

kumaraditya303 added a commit to kumaraditya303/cpython that referenced this pull request Jan 22, 2023
@kumaraditya303
Revert "pythongh-100795: avoid unexpected freeaddrinfo after failed…
285ad0d 
… `getaddrinfo` (python#101220)"

This reverts commit 5f08fe4.
@kumaraditya303 kumaraditya303 mentioned this pull request Jan 22, 2023
Merged
kumaraditya303 added a commit that referenced this pull request Jan 22, 2023
@kumaraditya303
Revert "gh-100795: avoid unexpected freeaddrinfo after failed `geta…
b53bad6 
…ddrinfo` (#101220)" (#101238)

Revert "gh-100795: avoid unexpected `freeaddrinfo` after failed `getaddrinfo` (#101220)"

This reverts commit 5f08fe4.
@sebres
Copy link
Contributor Author

sebres commented Jan 22, 2023

I'm sure that this PR is good, see my comment on the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead gpshead approved these changes

@arhadthedev arhadthedev arhadthedev left review comments

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Assignees
No one assigned
Labels
type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sebres @bedevere-bot @arhadthedev @miss-islington @kumaraditya303 @gpshead