GeezerVison is showing github.com — getaddrinfo raises OverflowError · Issue #74895 · python/cpython
getaddrinfo raises OverflowError #74895Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2017-06-20.13:19:55.914>
labels = ['extension-modules', 'type-bug', '3.7']
title = 'getaddrinfo raises OverflowError'
updated_at = <Date 2020-05-29.09:57:44.659>
user = 'https://github.com/smejkar' bugs.python.org fields:
activity = <Date 2020-05-29.09:57:44.659>
actor = 'remi.lapeyre'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2017-06-20.13:19:55.914>
creator = 'smejkar'
dependencies = []
files = ['46964']
hgrepos = []
issue_num = 30710
keywords = ['patch']
message_count = 11.0
messages = ['296421', '296437', '296471', '296473', '296526', '296536', '296540', '296541', '296548', '370283', '370285']
nosy_count = 5.0
nosy_names = ['benjamin.peterson', 'serhiy.storchaka', 'cheryl.sabella', 'smejkar', 'remi.lapeyre']
pr_nums = ['2435']
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30710'
versions = ['Python 3.7'] Linked PRs
Activitysmejkar commented on Jun 20, 2017 MannequinAuthor More actions If the port argument is a number, getaddrinfo attempts to convert it to a C long, that raises OverflowError if the conversion fails.
Instead, getaddrinfo should convert the port to a string (bytes) directly and rely on the underlying getaddrinfo to return EAI_SERVICE. This is also consistent with the case when a numeric port is passed as a string. serhiy-storchaka commented on Jun 20, 2017 What do you mean by converting the port to a string (bytes) directly? Can you provide an example? smejkar commented on Jun 20, 2017 MannequinAuthor More actions Use PyObject_Str and PyUnicode_AsUTF8 if the port argument is a PyLong instead of converting it to an intermediate C long that may raise OverflowError. See getaddrinfo_overflow_error.patch serhiy-storchaka commented on Jun 20, 2017 Now I understand your idea. It looks reasonable to me. Could you create a pull request on GitHub? Please provide also tests, add a Misc/NEWS entry and add your name in Misc/ACKS.
But how large is the performance hit of this change? It adds at least one additional memory allocation for the str object. If the slowdown is significant perhaps it is worth to keep the old code as a fast path. benjaminp commented on Jun 21, 2017 Why can't the user simply pass in a string service name in the first place? smejkar commented on Jun 21, 2017 MannequinAuthor More actions
But how large is the performance hit of this change? It adds at least one additional memory allocation for the str object. If the slowdown is significant perhaps it is worth to keep the old code as a fast path.
commit 5cc7ac2
./python -m timeit -s 'import socket' 'socket.getaddrinfo(None, 1000)'
2000 loops, best of 5: 139 usec per loop
./python -m timeit -s 'import socket' 'socket.getaddrinfo(None, "1000")'
2000 loops, best of 5: 142 usec per loop
with getaddrinfo_overflow_error.patch
./python -m timeit -s 'import socket' 'socket.getaddrinfo(None, 1000)'
2000 loops, best of 5: 140 usec per loop
./python -m timeit -s 'import socket' 'socket.getaddrinfo(None, "1000")'
2000 loops, best of 5: 139 usec per loop
It seems the impact on performance is negligible/not measurable. serhiy-storchaka commented on Jun 21, 2017 I like your idea about getting rid of OverflowError. But wouldn't it make the problem with other reported by you issue, bpo-30711, worse? smejkar commented on Jun 21, 2017 MannequinAuthor More actions
Why can't the user simply pass in a string service name in the first place?
He/she can. But I don't propose to change that. The patch only changes the way a given number is converted to a string. That is, without an intermediate C long.
smejkar commented on Jun 21, 2017 MannequinAuthor More actions
I like your idea about getting rid of OverflowError. But wouldn't it make the problem with other reported by you issue, bpo-30711, worse?
It depends on the implementation of the underlying getaddrinfo. For Modules/getaddrinfo.c, it will be worse for positive numbers outside the C long range. csabella commented on May 29, 2020 The original pull request has been closed as inactive. If the original author is interested in pursuing this issue, it can be reopened or someone else can create a new pull request to replace it. remilapeyre commented on May 29, 2020 Hi Cheryl Sabella, the pull request that got closed is the one for bpo-30711. The pull request for this one has never been reviewed. gpshead commented on Feb 14, 2023 thanks! Fixed to just rely on the underlying C library API for an error indicator or not. I'm not backporting this bugfix as it is very minor and changes in exception behavior are API changes that can be surprising, even if we don't think anything should be relying on it.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|