Page MenuHome GnuPG

server socket/pipe handling in GnuPG
Closed, ResolvedPublic


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

Revisions and Commits

Event Timeline

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);
               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);
                 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

The patch I proposed was partial one, not fully solved the problem of socket resource leak on Windows.

This time, I review the whole code in scdaemon.c.

It looks larger change in text, but the effect is actually: don't use close for socket allocated by assuan_sock_new (or npth_accept) but assuan_sock_close.

Also, I confirmed the code in libassuan, to see possible side effect. No side effect by the changes. When it's marked as cygwin use (not the case for GnuPG at the moment), it will be properly handled.

gniibe removed a project: Restricted Project.