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
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
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 with socket.gethostbyname() before connecting. That function is IPv4-only, so an ftp:// 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 to connect_ftp(), then to ftpwrapper, and finally to ftplib.FTP.connect(), which connects through socket.create_connection() and resolves via getaddrinfo(). 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.gaierror from gethostbyname() and was turned into URLError. With the pre-resolution gone, the same gaierror is raised deeper, inside ftplib. Because socket.gaierror is a subclass of OSError and therefore a member of ftplib.all_errors, ftp_open()'s existing handler still catches it and re-raises it as URLError, so callers see exactly the same exception type as before.
Tests: test_ftp now asserts the original hostname reaches connect_ftp() rather than a pre-resolved IP; test_ftp_no_hostname_resolution tripwires socket.gethostbyname to prove ftp_open() never calls it; and test_ftp_gaierror asserts the gaierror -> URLError conversion still holds. All three fail on main and pass with this change.