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

implementing socketToFd #424

Merged
merged 3 commits into from Sep 17, 2019
Merged

Conversation

kazu-yamamoto
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto commented Sep 3, 2019

@vdukhovni This is a PoC. I'm not sure that this implements what you want. Please give me your advice.

@kazu-yamamoto
Copy link
Collaborator Author

Originally discussed in #423.

socketToFd s = do
fd <- unsafeFdSocket s
-- FIXME: throw error no if -1
fd2 <- c_dup fd

Choose a reason for hiding this comment

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

[ IIRC, on windows INVALID_SOCKET is not -1 since SOCKET are unsigned on Windows. ]
Also, AFAIK, Windows does not have dup(2) for sockets, but it does have DuplicateHandle().
So making this portable requires may require some special-casing for Windows, unless suitably
special-cased wrappers are already available.

That said, yes, this is the general idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mistuke Would you suggest a proper way on Windows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think this may work fine. DuplicateHandle() is not correct for Sockets, it's explicitly called out not to do so on it's documentation page https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-duplicatehandle

You should not use DuplicateHandle to duplicate handles to the following objects:

   *  I/O completion ports. No error is returned, but the duplicate handle cannot be used.
   * Sockets. No error is returned, but the duplicate handle may not be recognized by
      Winsock a the target process. Also, using DuplicateHandle interferes with internal
     reference counting on the underlying object. To duplicate a socket handle, use the 
     WSADuplicateSocket function.

and also on the docs for Socket handles https://docs.microsoft.com/en-us/windows/win32/winsock/socket-handles-2.

But reason I think this may work is because on that same page it says

A socket handle can optionally be a file handle in Windows Sockets 2. A socket handle
from a Winsock provider can be used with other non-Winsock functions such as
ReadFile, WriteFile, ReadFileEx, and WriteFileEx.

So a socket can be a file handle in Winsock2, unsafeFdSocket is a call to _open_osfhandle which will return a descriptor for any system file handle, so should work with sockets. and so dup should work..

So easiest way is to test it and see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean we should merge this PR and release a new version and see what will happen?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, I mean write a simple test to see if the fd returned by this is usable. I can also use that to check if the associated kernel object's refcount decreases when you close it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can do both for you if you want but it'll have to wait for the weekend :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. I can wait. Thanks in advance!

@@ -162,6 +163,18 @@ withFdSocket (Socket ref _) f = do
touch ref
return r

-- | Socket is closed and a duplicated file descriptor is returned.
Copy link

@vdukhovni vdukhovni Sep 4, 2019

Choose a reason for hiding this comment

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

The documentation can then say, that the duplicated descriptor is no longer subject to the possibility of unexpectedly being closed if the socket is finalized. It is now the caller's responsibility to ultimately close the duplicated file descriptor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks and done.

@kazu-yamamoto
Copy link
Collaborator Author

@Mistuke Ping.

@Mistuke
Copy link
Collaborator

Mistuke commented Sep 16, 2019

Ah sorry! , was/am away for work. I'm traveling back today and will take care of this tomorrow after I land.

@Mistuke
Copy link
Collaborator

Mistuke commented Sep 17, 2019

This should work for Windows

diff --git a/Network/Socket/Types.hsc b/Network/Socket/Types.hsc
index 54b8744..bcb4295 100644
--- a/Network/Socket/Types.hsc
+++ b/Network/Socket/Types.hsc
@@ -174,6 +174,16 @@ withFdSocket (Socket ref _) f = do
 --   duplicated file descriptor.
 socketToFd :: Socket -> IO CInt
 socketToFd s = do
+#if defined(mingw32_HOST_OS)
+    fd <- unsafeFdSocket s
+    fd2 <- c_wsaDuplicate fd
+    -- FIXME: throw error no if -1
+    close s
+    return fd2
+
+foreign import ccall unsafe "wsaDuplicate"
+   c_wsaDuplicate :: CInt -> IO CInt
+#else
     fd <- unsafeFdSocket s
     -- FIXME: throw error no if -1
     fd2 <- c_dup fd
@@ -182,6 +192,7 @@ socketToFd s = do

 foreign import ccall unsafe "dup"
    c_dup :: CInt -> IO CInt
+#endif

 -- | Creating a socket from a file descriptor.
 mkSocket :: CInt -> IO Socket
diff --git a/cbits/initWinSock.c b/cbits/initWinSock.c
index e2e8008..e720b85 100644
--- a/cbits/initWinSock.c
+++ b/cbits/initWinSock.c
@@ -40,4 +40,19 @@ initWinSock ()
   return 0;
 }

+SOCKET
+wsaDuplicate (SOCKET s)
+{
+  WSAPROTOCOL_INFOW protocolInfo;
+  if (WSADuplicateSocketW (s, GetCurrentProcessId (), &protocolInfo) != 0)
+    return -1;
+
+  SOCKET res = WSASocketW(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO,
+                          FROM_PROTOCOL_INFO, &protocolInfo, 0, 0);
+  if (res == SOCKET_ERROR)
+    return -1;
+
+  return res;
+}
+
 #endif
diff --git a/tests/Network/SocketSpec.hs b/tests/Network/SocketSpec.hs
index 262a412..9bea422 100644
--- a/tests/Network/SocketSpec.hs
+++ b/tests/Network/SocketSpec.hs
@@ -209,3 +209,13 @@ spec = do
                     threadDelay 10000
                     void $ recv sock 1024
             tcpTest client server
+
+    describe "socketToFd" $ do
+        it "socketToFd can send using fd" $ do
+            let server sock = do
+                    void $ recv sock 1024
+                client sock = do
+                    fd <- socketToFd sock
+                    s <- mkSocket fd
+                    sendAll s "HELLO WORLD"
+            tcpTest client server

Credit: Tamar Christina <tamar@zhox.com>
kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Sep 17, 2019
@kazu-yamamoto kazu-yamamoto merged commit 27d9815 into haskell:master Sep 17, 2019
@kazu-yamamoto kazu-yamamoto deleted the socket2fd branch September 17, 2019 23:45
@kazu-yamamoto
Copy link
Collaborator Author

@Mistuke Thank you for your excellent work! This PR has been merged.

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 this pull request may close these issues.

None yet

3 participants