Can somebody pinch me, please?

[Corinna Vinschen <vinschen at redhat dot com>]

Guys,

While I was brooding over the mysterious ssh-add/ssh-agent hang, I
suddenly came across these fairly old lines in fhandler_socket::accept():

  if ((SOCKET) res == INVALID_SOCKET && WSAGetLastError () == WSAEWOULDBLOCK)
    in_progress = true;

Then, in case of unix stream sockets, the secret event stuff and the
new eid credential transaction is called.

Fine...

Looks good...

Well...

Hang on...

Erm...

What the heck is an accept in progress?!?  I mean, I understand a
non-blocking connect in progress which returns EWOULDBLOCK, but an
accept in progress?  What would be returned in the sockaddr parameter
if the return code is -1?  That doesn't make sense.

After months of asceticism I had another look into the SUSv3 and MSDN
man pages of accept

http://www.opengroup.org/onlinepubs/009695399/functions/accept.html
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/accept_2.asp (*)

and it turns out that returning -1 and an error of (WSA)EWOULDBLOCK
indicate that a non-blocking accept has simply nothing to accept!

So there is nobody knocking on the door (==connecting), which means,
calling any secret event or eip credential transaction is rightout
wrong.  Right?

I was going to apply the below patch.  I can confirm that ssh and (sigh)
ssh-agent/ssh-add still work for me (all are using non-blocking sockets),
but somehow I need a bit of shoulder patting before doing this.

Does anybody have problems or can imagine problems with this patch?
It just removes all in_progress handling from unix stream sockets, so
that the secret event and eid credential transactions only take place if
actually a socket connection has been accepted.


Anybody?

Corinna

(*) Isn't it funny that the MSDN accept man page is called "accept_2"?


	* fhandler_socket.cc (fhandler_socket::connect): Always set
	sun_path in case of a successful or pending connect.
	(fhandler_socket::accept): Don't run secret event and
	eid credential transactions if OS accept returned WSAEWOULDBLOCK

Index: fhandler_socket.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler_socket.cc,v
retrieving revision 1.153
diff -u -p -r1.153 fhandler_socket.cc
--- fhandler_socket.cc	9 Mar 2005 23:00:20 -0000	1.153
+++ fhandler_socket.cc	10 Mar 2005 11:31:45 -0000
@@ -637,6 +637,10 @@ fhandler_socket::connect (const struct s
 	}
       set_winsock_errno ();
     }
+
+  if (get_addr_family () == AF_LOCAL && (!res || in_progress))
+    set_sun_path (name->sa_data);
+
   if (get_addr_family () == AF_LOCAL && get_socket_type () == SOCK_STREAM)
     {
       if (!res || in_progress)
@@ -670,12 +674,12 @@ fhandler_socket::connect (const struct s
       if (!res || in_progress)
         {
 	  /* eid credential transaction. */
-	  set_sun_path (name->sa_data);
 	  if (wincap.has_named_pipes ())
 	    {
 	      struct ucred in = { getpid (), geteuid32 (), getegid32 () };
 	      struct ucred out = { (pid_t) 0, (__uid32_t) -1, (__gid32_t) -1 };
 	      DWORD bytes = 0;
+	      debug_printf ("Calling CallNamedPipe");
 	      if (CallNamedPipe(eid_pipe_name ((char *) alloca (CYG_MAX_PATH + 1)),
 				&in, sizeof in, &out, sizeof out, &bytes, 1000))
 		{
@@ -745,7 +749,6 @@ fhandler_socket::accept (struct sockaddr
 {
   int res = -1;
   bool secret_check_failed = false;
-  bool in_progress = false;
   struct ucred in = { sec_pid, sec_uid, sec_gid };
   struct ucred out = { (pid_t) 0, (__uid32_t) -1, (__gid32_t) -1 };
 
@@ -769,21 +772,13 @@ fhandler_socket::accept (struct sockaddr
 
   res = ::accept (get_socket (), peer, len);
 
-  if ((SOCKET) res == INVALID_SOCKET && WSAGetLastError () == WSAEWOULDBLOCK)
-    in_progress = true;
-
-  if (get_addr_family () == AF_LOCAL && get_socket_type () == SOCK_STREAM)
+  if ((SOCKET) res != INVALID_SOCKET && get_addr_family () == AF_LOCAL
+      && get_socket_type () == SOCK_STREAM)
     {
-      if ((SOCKET) res != INVALID_SOCKET || in_progress)
-	{
-	  if (!create_secret_event ())
-	    secret_check_failed = true;
-	  else if (in_progress)
-	    signal_secret_event ();
-	}
+	if (!create_secret_event ())
+	  secret_check_failed = true;
 
-      if (!secret_check_failed &&
-	  (SOCKET) res != INVALID_SOCKET)
+      if (!secret_check_failed)
 	{
 	  if (!check_peer_secret_event ((struct sockaddr_in*) peer))
 	    {
@@ -795,40 +790,37 @@ fhandler_socket::accept (struct sockaddr
       if (secret_check_failed)
 	{
 	  close_secret_event ();
-	  if ((SOCKET) res != INVALID_SOCKET)
-	    closesocket (res);
+	  closesocket (res);
 	  set_errno (ECONNABORTED);
 	  return -1;
 	}
 
-      if ((SOCKET) res != INVALID_SOCKET || in_progress)
-        {
-	  /* eid credential transaction. */
-	  if (wincap.has_named_pipes ())
+      /* eid credential transaction. */
+      if (wincap.has_named_pipes ())
+	{
+	  DWORD bytes = 0;
+	  debug_printf ("Calling ConnectNamedPipe");
+	  bool ret = ConnectNamedPipe (sec_pipe, NULL);
+	  if (ret || GetLastError () == ERROR_PIPE_CONNECTED)
 	    {
-	      DWORD bytes = 0;
-	      bool ret = ConnectNamedPipe (sec_pipe, NULL);
-	      if (ret || GetLastError () == ERROR_PIPE_CONNECTED)
-		{
-		  if (!ReadFile (sec_pipe, &out, sizeof out, &bytes, NULL))
-		    debug_printf ("Receiving eid credentials failed: %E");
-		  else
-		    debug_printf ("Received eid credentials: pid: %d, uid: %d"
-		    		  ", gid: %d", out.pid, out.uid, out.gid);
-		  if (!WriteFile (sec_pipe, &in, sizeof in, &bytes, NULL))
-		    debug_printf ("Sending eid credentials failed: %E");
-		  DisconnectNamedPipe (sec_pipe);
-		}
+	      if (!ReadFile (sec_pipe, &out, sizeof out, &bytes, NULL))
+		debug_printf ("Receiving eid credentials failed: %E");
 	      else
-		debug_printf ("Connecting the eid credential pipe failed: %E");
-	    }
-	  else /* 9x */
-	    {
-	      /* Incorrect but wrong pid at least doesn't break getpeereid. */
-	      out.pid = sec_pid;
-	      out.uid = sec_uid;
-	      out.gid = sec_gid;
+		debug_printf ("Received eid credentials: pid: %d, uid: %d"
+			      ", gid: %d", out.pid, out.uid, out.gid);
+	      if (!WriteFile (sec_pipe, &in, sizeof in, &bytes, NULL))
+		debug_printf ("Sending eid credentials failed: %E");
+	      DisconnectNamedPipe (sec_pipe);
 	    }
+	  else
+	    debug_printf ("Connecting the eid credential pipe failed: %E");
+	}
+      else /* 9x */
+	{
+	  /* Incorrect but wrong pid at least doesn't break getpeereid. */
+	  out.pid = sec_pid;
+	  out.uid = sec_uid;
+	  out.gid = sec_gid;
 	}
     }
 

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          mailto:cygwin@cygwin.com
Red Hat, Inc.