crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Unable to run compiler or stdlib specs with IPv6 disabled #15033

Closed Blacksmoke16 closed 3 days ago

Blacksmoke16 commented 1 month ago

At some point in the past while debugging something else, I ended up adding the ipv6.disable=1 kernel parameter. This seems to break the compiler and stdlib specs due to:

Unhandled exception: Failed to create socket: Address family not supported by protocol (Socket::Error)
  from src/crystal/system/unix/socket.cr:12:5 in 'create_handle'
  from src/socket.cr:68:10 in 'initialize'
  from src/socket/tcp_socket.cr:40:5 in 'initialize'
  from src/socket/tcp_server.cr:38:7 in 'initialize:reuse_port'
  from src/socket/tcp_server.cr:36:3 in 'new:reuse_port'
  from src/socket/tcp_server.cr:70:14 in 'supports_ipv6?'
  from spec/std/socket/spec_helper.cr:26:6 in 'each_ip_family'
  from spec/std/http/web_socket_spec.cr:360:3 in '->'
  from src/spec/context.cr:286:15 in 'describe'
  from src/spec/methods.cr:20:5 in 'describe'
  from spec/std/http/web_socket_spec.cr:47:1 in '__crystal_main'
  from src/crystal/main.cr:118:5 in 'main_user_code'
  from src/crystal/main.cr:104:7 in 'main'
  from src/crystal/main.cr:130:3 in 'main'
  from /usr/lib/libc.so.6 in '??'
  from /usr/lib/libc.so.6 in '__libc_start_main'
  from .build/std_spec in '_start'
  from ???
make: *** [Makefile:106: std_spec] Error 1

We are checking if IPv6 is enabled via SocketSpecHelper.supports_ipv6?, but we're only catching Socket::BindError whereas the error raised here is a Socket::Error. Will also need to address unused_local_port always using IPv6 as well.

Blacksmoke16 commented 1 month ago

Some networking specs do https://github.com/crystal-lang/crystal/blob/9df6760292f525d4a35c243f1d76e7ab534cec44/spec/std/openssl/ssl/server_spec.cr#L14 which ends up using "::" as the host. These specs ultimately fail when IPv6 is disabled. Is there any meaningful difference between "::" and "0.0.0.0" in this context? Was thinking we could update these usages to use the IPv4 variant to avoid needing extra IPv6 checks.

ysbaddaden commented 1 month ago

Yes :: binds to ipv6 and ipv4, while 0.0.0.0 only binds to ipv4.

ysbaddaden commented 1 month ago

We want the former behavior by default, not the latter.

I'm wondering in which context is ipv6 disabled? I mean, we're in 2024.

Blacksmoke16 commented 1 month ago

We want the former behavior by default, not the latter.

Yea that makes sense in the real code, but to be clear I was more so proposing just changing it in these specs. E.g.

it "don't sync_close" do
  TCPServer.open(0) do |tcp_server|
    context = OpenSSL::SSL::Context::Server.new
    ssl_server = OpenSSL::SSL::Server.new(tcp_server, context, sync_close: false)
    ssl_server.context.should eq context

    ssl_server.close

    tcp_server.closed?.should be_false
  end
end

Does it really matter if we bind an IPv6 vs IPv4 in this context?

straight-shoota commented 1 month ago

It appears the behaviour of :: can vary, as previously mentioned in https://github.com/crystal-lang/crystal/pull/6711#issuecomment-421030196. I suppose this might be the case in @Blacksmoke16's environment that :: does not include ipv4 as well and thus the bind fails with ipv6 disabled?

Anyway, I'm wondering if using an unspecified address is even good in the first place. Shouldn't these servers bind to loopback instead?