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

bug(transport): it should not try to resolve sockaddrs when using build_with_stream #1411

Closed
oleonardolima opened this issue Jun 18, 2024 · 2 comments · Fixed by #1412
Closed

Comments

@oleonardolima
Copy link
Contributor

oleonardolima commented Jun 18, 2024

I've been using and testing the build_with_stream method added in #1168, but as Target been used in both build_with_stream and try_connect it's resolving the sockaddrs when creating Target from the given Url, and leading to two problems:

  • it's possibly leaking data as it resolves through a standard connection and not through the preferred data stream passed as a parameter.
  • in connections, to .onion for example, it leads to an error because it tries to resolve it to sockaddrs which is expected to not be possible, and it's one of the reasons to use the specified data stream, in this case for example arti_client::DataStream.

I do see two possible fixes for the problem:

  • remove the sockaddrs resolution from the Target data structure, and do it only on try_connect_over_tcp, which is the only place it's being used.
  • receive the Target as a parameter, with the sockaddrs field being an Option<> leading to the user to possibly do the resolution when needed. It would also require upstreaming the parameter to ws-client methods as well.
@niklasad1
Copy link
Member

niklasad1 commented Jun 18, 2024

remove the sockaddrs resolution from the Target data structure, and do it only on try_connect_over_tcp, which is the only place it's being used.

That's the preferred option if possible and I don't want to expose elsewhere if possible.

@oleonardolima
Copy link
Contributor Author

remove the sockaddrs resolution from the Target data structure, and do it only on try_connect_over_tcp, which is the only place it's being used.

That's the preferred option if possible and I don't want to expose elsewhere if possible.

Sure, I worked on that approach on #1412, please let me know what you think.

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

Successfully merging a pull request may close this issue.

2 participants