Skip to content
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.

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

bpo-35545: Fix mishandling of scoped IPv6 addresses #11403

Closed

Conversation

twisteroidambassador
Copy link
Contributor

@twisteroidambassador twisteroidambassador commented Jan 2, 2019

Make sure _ensure_resolved() is only called on user-provided (host, port), and not on already-resolved socket address tuples.

This should also fix bpo-33678.

https://bugs.python.org/issue35545

High-level APIs such as create_connection() accept (host, port), while
low-level APIs such as sock_connect() accept socket address tuples.
_ensure_resolved() should be used exactly once to turn (host, port)
into an address tuple.

The signature of _ensure_resolved() was changed from (address, *, ...)
to (host, port, *, ...) to enforce this.
@1st1
Copy link
Member

1st1 commented Feb 14, 2019

@asvetlov please review.

@csabella
Copy link
Contributor

@1st1, is this PR still needed since the other one for the issue was merged? Thanks!

@1st1
Copy link
Member

1st1 commented May 25, 2020

I'm not sure. @ @twisteroidambassador do we still need this one? Do you want it in 3.9beta 2?

@twisteroidambassador
Copy link
Contributor Author

twisteroidambassador commented May 26, 2020

Most likely this PR is not needed anymore.

This PR is associated with 2 issues, bpo-33678 and bpo-35545. bpo-35545 has two PRs, and that issue has been fixed with the other PR #11271 that got merged.

(That PR went in a completely different direction from this one. The root cause of bpo-35545 is that, when a pre-resolved link-local IPv6 addresses pass through _ensure_resolved() again, information is lost. My idea is to make sure an address never passes through _ensure_resolved() more than once. I still think this is the cleaner approach, but it is a breaking change, because it means sock_connect() no longer accepts all (host, port), but must take a pre-resolved address. #11271 instead made sure that _ensure_resolved() does not lose information, so it fixed the issue in a backward-compatible way.)

bpo-33678 is not fixed yet. This PR would fix it as a side effect, but since we already went with #11271 for bpo-35545, I don't think we should merge this one just for bpo-33678. Instead, I think bpo-33678 can be fixed in a minimally-intrusive way as suggested by the first comment there, basically changing this line:

resolved = await self._ensure_resolved(
address, family=sock.family, proto=sock.proto, loop=self)

To:

        resolved = await self._ensure_resolved(
            address, family=sock.family, type=sock.type, proto=sock.proto, loop=self)

@1st1
Copy link
Member

1st1 commented May 26, 2020

Do you want to close this PR and submit a new one?

@twisteroidambassador
Copy link
Contributor Author

I don't have time to make a new PR lately, but I agree to close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants