Skip to content

gh-74895: getaddrinfo no longer raises OverflowError - #2435

Merged
gpshead merged 9 commits into
python:mainfrom
smejkar:bpo-30710
Feb 14, 2023
Merged

gh-74895: getaddrinfo no longer raises OverflowError#2435
gpshead merged 9 commits into
python:mainfrom
smejkar:bpo-30710

Conversation

@smejkar

@smejkarsmejkar commented Jun 27, 2017•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@smejkar
bpo-30710: getaddrinfo does not raise OverflowError
fe317a4
Convert a numeric port number to a string without an intermediate C long.
This prevents raising OverflowError if the port number is out of the
C long range.
@smejkar

smejkar commented Jun 27, 2017

Copy link
Copy Markdown
ContributorAuthor

@serhiy-storchaka to pass checks, should I (a) wait till #2436 is merged, (b) relax test_getaddrinfo_overflow, or (c) rebase this onto #2436?

MaxwellDupre
MaxwellDupre suggested changes Feb 18, 2022
View reviewed changes

@MaxwellDupreMaxwellDupre left a comment

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.

Test output:
`FAIL: test_getaddrinfo_overflow (test.test_socket.GeneralModuleTests)

Traceback (most recent call last):
File "/home/me/Documents/cpython/Lib/test/test_socket.py", line 1370, in test_getaddrinfo_overflow
socket.getaddrinfo(None, _testcapi.LONG_MAX + 1)
AssertionError: gaierror not raised
`
Please fix.

@github-actions

github-actionsBot commented Mar 21, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Mar 21, 2022
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022
@github-actions

github-actionsBot commented Aug 29, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Aug 29, 2022
@arhadthedevarhadthedev changed the title bpo-30710: getaddrinfo raises OverflowErrorgh-74895: getaddrinfo raises OverflowErrorFeb 13, 2023
@arhadthedevarhadthedev added extension-modules C modules in the Modules dir topic-IO labels Feb 13, 2023
@arhadthedev

arhadthedev commented Feb 13, 2023

Copy link
Copy Markdown
Member

@giampaolo, @gpshead (as network experts)

@smejkarsmejkarmannequin mentioned this pull request Feb 13, 2023
Closed
@gpsheadgpshead self-assigned this Feb 13, 2023
@gpshead
Merge branch 'main' into bpo-30710
399c274
@gpshead
use out of range service/port values across all platforms
2ec8318
Some systems treat it as unsigned internally so ULONG_MAX+1 is needed, not LONG_MAX+1.
@gpsheadgpshead added the type-bug An unexpected behavior, bug, or error label Feb 13, 2023
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Feb 14, 2023
@gpshead
Rework the test to allow gaierror or no-error.
7328057
Also updates our canned getaddrinfo.c implementation to actually
report an error for values outside of the u_short range it casts
the value to instead of ignoring them.
@gpsheadgpshead changed the title gh-74895: getaddrinfo raises OverflowErrorgh-74895: getaddrinfo no longer raises OverflowErrorFeb 14, 2023
@gpshead

gpshead commented Feb 14, 2023

Copy link
Copy Markdown
Member

@serhiy-storchaka to pass checks, should I (a) wait till #2436 is merged, (b) relax test_getaddrinfo_overflow, or (c) rebase this onto #2436?

I effectively reworked the test, we don't actually want an error behavior on specific numeric values as we had in the past, we just want to report and error if the underlying platform's getaddrinfo() call reports an error. Not all of them do, or at least not all at the same values.

for all practical purposes I doubt it matters much. it'd be nice for values outside of 16-bit unsigned to be rejected, but that only makes strict sense for IPv4 and IPv6. Other protocols are free to treat numeric service names (aka port) differently.

@gpshead
gpshead merged commit 928752c into python:mainFeb 14, 2023
@kulikjakkulikjak mentioned this pull request May 9, 2023
Merged
@CendioHalimCendioHalim mentioned this pull request Dec 27, 2023
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpsheadgpsheadgpshead approved these changes
+1 more reviewer
@MaxwellDupreMaxwellDupreMaxwellDupre requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@gpsheadgpshead

Labels

extension-modulesC modules in the Modules dirtopic-IOtype-bugAn unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@smejkar@arhadthedev@gpshead@MaxwellDupre@brettcannon@Mariatta@the-knights-who-say-ni@ezio-melotti@bedevere-bot