I found that there are some differences between gpg-agent and other daemons.
If following gpg-agent is better, we need to:
Description
Description
Revisions and Commits
Revisions and Commits
| rG GnuPG | |||
| rGa67a09be30f5 scd,w32: Fix socket resource leak. | |||
Related Objects
Related Objects
- Mentioned In
- T5743: Release GnuPG 2.3.5
Event Timeline
Comment Actions
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.
Comment Actions
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);
Comment Actions
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.
Comment Actions
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
Comment Actions
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.