server socket/pipe handling in GnuPG
Open, NormalPublic

Description

I found that there are some differences between gpg-agent and other daemons.

If following gpg-agent is better, we need to:

  • Use gnupg_fd_t for fd of listening
  • close should be assuan_sock_close
    • Perhaps, use of close on pipe is wrong on W32
  • Return value of npth_accept should be converted by INT2FD
    • IIUC, calling assuan_sock_close (instead of close) on W32 with the fd returned by npth_accept is OK

Related Objects

gniibe created this task.Aug 19 2020, 10:10 AM
gniibe added a project: gnupg.
werner added a subscriber: werner.Aug 19 2020, 1:04 PM

I's say we should not do anything but solve that along with the move of all fd/fp/sock/HANDLE stuff to gpgrt to solve this at one place. We need that anyway to properly support Windows64. We won't be abale to do this for 2.3, though.

I mean these uses of close:

diff --git a/scd/scdaemon.c b/scd/scdaemon.c
index b7bbc0361..a6925eaf9 100644
--- a/scd/scdaemon.c
+++ b/scd/scdaemon.c
@@ -797,8 +797,8 @@ main (int argc, char **argv )
       /* We run handle_connection to wait for the shutdown signal and
          to run the ticker stuff.  */
       handle_connections (fd);
-      if (fd != -1)
-        close (fd);
+      if (fd != GNUPG_INVALID_FD)
+        assuan_sock_close (fd);
     }
   else if (!is_daemon)
     {
@@ -932,7 +932,7 @@ main (int argc, char **argv )
 
       handle_connections (fd);
 
-      close (fd);
+      assuan_sock_close (fd);
     }
 
   xfree (config_filename);
@@ -1390,22 +1392,22 @@ handle_connections (int listen_fd)
             {
               log_error ("error allocating connection control data: %s\n",
                          strerror (errno) );
-              close (fd);
+              assuan_sock_close (fd);
             }
           else
             {
               char threadname[50];
               npth_t thread;
 
-              snprintf (threadname, sizeof threadname, "conn fd=%d", fd);
-              ctrl->thread_startup.fd = INT2FD (fd);
+              snprintf (threadname, sizeof threadname, "conn fd=%d", FD2INT (fd));
+              ctrl->thread_startup.fd = fd;
               ret = npth_create (&thread, &tattr, start_connection_thread, ctrl);
               if (ret)
                 {
                   log_error ("error spawning connection handler: %s\n",
                              strerror (ret));
                   xfree (ctrl);
-                  close (fd);
+                  assuan_sock_close (fd);
                 }
               else
                 npth_setname_np (thread, threadname);

It's pretty minor bug, it only matters for some strange scenario on Windows like:
(1) a user runs gpg --card-edit or gpg-card and keeps the user interaction for some reason (say, forgetting the terminal interaction), which keeps the pipe connection from gpg-agent to scdaemon
(2) While the pipe connection is used by the user interaction above, from another terminal, the user invokes gpg (say, gpg --decrypt) which uses socket connection from gpg-agent to scdaemon

Here, close fails, and the socket HANDLE is not released, thus, if the invocation of (2) continues many many times, I believe that eventually scdaemon will die because of no resource.

werner triaged this task as Normal priority.Aug 27 2020, 11:01 AM
werner edited projects, added gnupg (gpg23); removed gnupg.

I still don't think that it is correct. We would also need to turn fd from an int to a gnupg_fd_t (ie. a HANDLE under Windows) which requires other changes and should be done in the other parts of the code as well. assuan_sock_close also delegates to the system specific function and on Windows removes the fd also from the cygwin table. This may trigger other bugs so I'd like to keep it as it is to go with the code which has been in active use for a long time - at least for 2.2