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

Fix wrong default address when binding sockets #13006

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

etra0
Copy link
Contributor

@etra0 etra0 commented Jan 26, 2023

Hello! This is my first PR!

The following code would crash on my laptop:

require "socket"

entry = TCPSocket.new
entry.bind(1234)

With the stacktrace

Unhandled exception: Hostname lookup for :: failed: No address found (Socket::Addrinfo::Error)
  from /opt/homebrew/Cellar/crystal/1.7.2/share/crystal/src/socket/addrinfo.cr:132:7 in 'bind'
  from /opt/homebrew/Cellar/crystal/1.7.2/share/crystal/src/socket/ip_socket.cr:23:5 in 'bind'
  from smth.cr:4:1 in '__crystal_main'

Because it tries to resolve the address :: when the Family is INET.

I am unsure on raising an exception because that case should never hit since you cannot initiate an unix socket by only assigning a port, advice is welcomed!

@straight-shoota
Copy link
Member

Thanks for your contribution! Looks good 👍

About the error case: Currently you'll get an exception with message Failed to create socket: Protocol not supported when calling this method with a non-inet family:

entry = TCPSocket.new(family: :unix)
entry.bind(1234) # SocketError: Failed to create socket: Protocol not supported

I think that's what should happen after this patch as well. Now we could handle this case explicitly here, or just pass it through. An exception will be raise eventually.
So maybe the address could just be @family.inet? ? "0.0.0.0:#{port}" : "::#{port}", effectively just changing the current behaviour for INET family.

Note that directly interpolating the address and port is an optimization that avoids an intermediary string allocation.

@etra0
Copy link
Contributor Author

etra0 commented Jan 26, 2023

hey, thanks for the feedback!

FWIW I just tested the example you sent and it still raises the exception you mention, because that one is triggered in the .new method I think.

changes updated 😄

src/socket.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

Please avoid force pushes to active PRs. Instead, just append changes as new commits. See https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#making-good-pull-requests
Thanks.

Removed unused variable assignment in the unix socket test.
@etra0
Copy link
Contributor Author

etra0 commented Jan 26, 2023

Oh, I'm sorry for overwriting the history, should have caught that earlier when reading the contributing docs.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good 👍

src/socket.cr Outdated Show resolved Hide resolved
Although the `address_and_port` is not used in `system_bind`, I think
it's important to display a correct error message, and so the IPv6 is
enclosed in square brackets [1] in case the exception is triggered

[1]: https://datatracker.ietf.org/doc/html/rfc2732#section-2
@etra0 etra0 requested a review from Sija January 27, 2023 20:46
Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@etra0
Copy link
Contributor Author

etra0 commented Jan 29, 2023

I don't think the failures are related to this PR. One is from OAuth, and the other one to bind SSL server but from the stacktrace it doesn't seem to touch any of my changes.

@straight-shoota straight-shoota added this to the 1.8.0 milestone Feb 13, 2023
@straight-shoota straight-shoota merged commit 9bed917 into crystal-lang:master Feb 15, 2023
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

4 participants