gh-44672: Stop pre-resolving the FTP host in urllib.request#154192
Open
soreavis wants to merge 1 commit into
Open
gh-44672: Stop pre-resolving the FTP host in urllib.request#154192soreavis wants to merge 1 commit into
soreavis wants to merge 1 commit into
Conversation
FTPHandler.ftp_open() resolved the host with socket.gethostbyname() before connecting. gethostbyname() is IPv4-only, so an ftp:// URL for an IPv6-only host failed there even when the server was reachable. As diagnosed on the tracker in 2009, ftp_open() has no need to resolve the host itself: it is passed to connect_ftp(), then to ftpwrapper, and on to ftplib.FTP.connect(). That uses socket.create_connection(), which resolves via getaddrinfo() and is IPv6-aware. Remove the pre-resolution and let the host flow through unresolved. An unresolvable host now raises socket.gaierror inside ftplib; gaierror is an OSError and therefore in ftplib.all_errors, so ftp_open()'s existing handler still turns it into URLError and callers see the same error type as before.
20 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
urllib.request.FTPHandler.ftp_open()pre-resolves the host withsocket.gethostbyname()before connecting. That function is IPv4-only, so anftp://URL pointing at an IPv6-only host fails at the resolve step with a name-resolution error even though the server is reachable over IPv6.As diagnosed on the tracker (bpo-1675455) back in 2009,
ftp_open()has no need to resolve the host itself: it hands the host toconnect_ftp(), then toftpwrapper, and finally toftplib.FTP.connect(), which connects throughsocket.create_connection()and resolves viagetaddrinfo(). That path is already IPv6-aware. This change removes the pre-resolution so the host flows through unresolved and IPv6-only hosts work.The one behavior to preserve is the exception type for a bad host. Previously an unresolvable host raised
socket.gaierrorfromgethostbyname()and was turned intoURLError. With the pre-resolution gone, the samegaierroris raised deeper, insideftplib. Becausesocket.gaierroris a subclass ofOSErrorand therefore a member offtplib.all_errors,ftp_open()'s existing handler still catches it and re-raises it asURLError, so callers see exactly the same exception type as before.Tests:
test_ftpnow asserts the original hostname reachesconnect_ftp()rather than a pre-resolved IP;test_ftp_no_hostname_resolutiontripwiressocket.gethostbynameto proveftp_open()never calls it; andtest_ftp_gaierrorasserts thegaierror->URLErrorconversion still holds. All three fail onmainand pass with this change.