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

socket raises exception (un)setting IPv6Only #180

Closed
TravisCardwell opened this issue Sep 8, 2015 · 11 comments
Closed

socket raises exception (un)setting IPv6Only #180

TravisCardwell opened this issue Sep 8, 2015 · 11 comments

Comments

@TravisCardwell
Copy link
Contributor

In Network.Socket:socket, the IPv6Only socket option is set to 0 so that both IPv4 and IPv6 can be handled with one socket, when the address family is AF_INET6. This is not always desired, and some parameters cause an exception to be raised.

For example, ICMPv6 (protocol number 58) is a protocol that is specifically for IPv6. The issue can be reproduced in GHCi on Linux as follows:

> import Network.Socket
> sock <- socket AF_INET6 Raw 58
*** Exception: setSocketOption: invalid argument (Invalid argument)

The code already suppresses setSocketOption errors on Windows platforms. A simple fix would be to do so on all platforms, and I am preparing a pull request for this. Personally, I feel that it would be preferable to not set the option in socket, especially since its appropriateness is application-specific and will likely change as IPv4 becomes less prominent.

@TravisCardwell
Copy link
Contributor Author

@kazu-yamamoto
Hibino-san suggested that I let you know that I created this issue. I am new to ICMPv6 programming, so please feel free to let me know if I am approaching it incorrectly.

By the way, (the C equivalent of) the above example is in the iputils ping6 implementation:

icmp_sock = socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6);

@kazu-yamamoto
Copy link
Collaborator

I don't think it is wise to ignore errors.
I think we should set IPv6Only only when the second option is Stream and Datagram.

@tibbe
Copy link
Member

tibbe commented Sep 10, 2015

I agree with Kazu. Unconditionally ignoring all errors is not a good idea.

@kazu-yamamoto
Copy link
Collaborator

I would like to change the code like this:

#if HAVE_DECL_IPV6_V6ONLY
# if Unix or Windows Vista or later
    when (family == AF_INET6 &&
         (stype == Stream || stype == Datagram)) $
       setSocketOption sock IPv6Only 0
# endif
#endif

Is there any way to check Vista or later?

@kazu-yamamoto
Copy link
Collaborator

Why HAVE_DECL_IPV6_V6ONLY is set for Windows prior to Vista?

@TravisCardwell
Copy link
Contributor Author

I also agree that ignoring errors is a bad idea. I have updated the pull request with code like Kazu's:
https://github.com/haskell/network/pull/181/files?diff=unified

I guess that simply removing the setSocketOption call is undesirable because of backwards compatibility with code that expects it? As long as it does not raise an exception, a program (using AF_INET6 with Stream or Datagram) can just reset the option (setSocketOption sock IPv6Only 1) after creating the socket. If an exception is raised, however, the program cannot acquire the socket at all. By the way, in that case, the lost socket is not closed; perhaps the setSocketOption call could use some exception handling?

@kazu-yamamoto
Copy link
Collaborator

In the current pull request, it seems to me that IPv6Only is not set to 0 on Vista or laster. Is this intentional? If I remember correctly, default value of IPV6_V6ONLY is platform specific. That's why we explicitly set it to 0 here.

@kazu-yamamoto
Copy link
Collaborator

I think that closing a socket on errors is a good idea. onException would help.

@TravisCardwell
Copy link
Contributor Author

That is my mistake; thank you for catching it!

I now better understand the reasoning behind this code. The IPV6_V6ONLY option should be turned off by default according to RFC 3493 Section 5.3, but it seems that some people disagree with the decision and implement it differently. (relevant blog post) I have added a comment to my pull request to document this.

It is indeed strange if HAVE_DECL_IPV6_V6ONLY is set on platforms that do not support the option. Investigating, I see in configure.ac that IPV6_V6ONLY is explicitly defined as a "fix for MingW not defining [it]."

I am unable to find a flag that could be used to indicate the Windows version. (GHC documentation) In my pull request, I am reverting to error suppression for Windows, as I assume that it was there for a reason and am unable to test it myself.

I also added an onException call to close the socket on error. Note that it is being imported from GHC.IO.Exception, as Control.Exception is only imported for Windows.

@kazu-yamamoto
Copy link
Collaborator

LGTM.

@tibbe Let's merge this.

@kazu-yamamoto
Copy link
Collaborator

Pinging @tibbe

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

No branches or pull requests

3 participants