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 TCPSocket segfault on missing host and 0 port #4177

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

will
Copy link
Contributor

@will will commented Mar 22, 2017

Prior to this patch, TCPSocket.new("whatever", 0) would cause
Invalid memory access (signal 11) at address 0x0

~ ➤ crystal --version
Crystal 0.21.1 (2017-03-14) LLVM 4.0.0
~ ➤ crystal eval 'require "socket"; require "socket/tcp_socket"; TCPSocket.new("hello", 0)'
Invalid memory access (signal 11) at address 0x0
[4486190123] *CallStack::print_backtrace:Int32 +107
[4486161591] __crystal_sigfault_handler +55
[140735515010346] _sigtramp +26
[140735492457349] _gai_simple +88
[140735492434315] search_addrinfo +179
[140735492419567] si_addrinfo +2255
[140735492417147] getaddrinfo +179
[4486349509] *TCPSocket#initialize<String, Int32, Nil, Nil>:Nil +709
[4486348787] *TCPSocket#initialize<String, Int32>:Nil +35
[4486348727] *TCPSocket::new<String, Int32>:TCPSocket +183
[4486139052] __crystal_main +1228
[4486161336] main +40
~ ➤ bcrystal eval 'require "socket"; require "socket/tcp_socket"; TCPSocket.new("hello", 0)'
Using compiled compiler at .build/crystal
No address found for hello:0 over TCP (Socket::Error)
0x104379885: *CallStack::unwind:Array(Pointer(Void)) at ??
0x104379821: *CallStack#initialize:Array(Pointer(Void)) at ??
0x1043797f8: *CallStack::new:CallStack at ??
0x104378631: *raise<Socket::Error>:NoReturn at ??
0x1043a6272: *TCPSocket#initialize<String, Int32, Nil, Nil>:Nil at ??
0x1043a5f53: *TCPSocket#initialize<String, Int32>:Nil at ??
0x1043a5f17: *TCPSocket::new<String, Int32>:TCPSocket at ??
0x104372bec: __crystal_main at ??
0x104378308: main at ??
~ ➤

@will will force-pushed the fix-tcp-segfault branch from c0de776 to c0de5c2 Compare March 22, 2017 23:21
@will
Copy link
Contributor Author

will commented Mar 22, 2017

Oops, I didn't run the whole specs. This is not the correct fix for the segfault maybe.

@will
Copy link
Contributor Author

will commented Mar 22, 2017

Hm, actually this looks like it might be a bug with mac's glibc: https://bugs.python.org/issue17269

@ysbaddaden
Copy link
Contributor

If it's specific to macOS, we should do the same as Python: add a small patch around getaddrinfo checking if port == 0 and not set AI_NUMERICSERV (or just return?) if that's the case.

Prior to this patch, `TCPSocket.new("whatever", 0)` would cause
    Invalid memory access (signal 11) at address 0x0
@will will force-pushed the fix-tcp-segfault branch from c0de5c2 to c0dee76 Compare March 24, 2017 01:31
@will
Copy link
Contributor Author

will commented Mar 24, 2017

@ysbaddaden yeah good idea. I gave that a shot now, but I'm now on a machine that is 10.12 and apparently they fixed libsystem on there, and I ran into this on 10.11. So I need to wait until I get home to see if it actually fixed the issue :)

Not sure if you want the patch since it's now fixed in the newest version of macOS/os x, but not everyone is always on the bleeding edge.

@ysbaddaden
Copy link
Contributor

not everyone is always on the bleeding edge

Exactly.

@will
Copy link
Contributor Author

will commented Mar 24, 2017

Ok, just tried this on my 10.11 and it does fix it there too. I think everything is good now.

@bcardiff bcardiff added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels Mar 27, 2017
@bcardiff bcardiff merged commit 3c74890 into crystal-lang:master Mar 27, 2017
@bcardiff bcardiff added this to the Next milestone Mar 27, 2017
@bcardiff
Copy link
Member

Thanks @will ! 🕵️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants