session BUGFIX do not close SSH socket twice

[Ilyesbenhamouda]

Currently, SSH socket is closed twice when created by libssh, causing undefined behavior in multithreaded programs (e.g. netopeer2-server).

ssh_disconnect() used to close the SSH socket. However, this behavior changed with libssh 0.10: the socket is no longer closed when passed through options.
Let's only close SSH socket when created by us after disconnecting from the session, if libssh >= 0.10 (libnetconf2 requires libssh >= 0.9.5).

[michalvasko]

Why did you add the opt_sock member into the session instead of keeping the socket in the libssh session and getting it using ssh_get_fd()?

[michalvasko]

Thanks for letting us know about this, the change went unnoticed. But, based on the docs the socket now needs to be closed explicitly. So they are not being closed twice but they are not closed at all, which is probably why it did not cause any problems.

[michalvasko]

Actually, I think it now works correctly for libssh >= 0.10.0, I do remember changing this a long time ago, but 0.10.0 is 3 year old. The issue you describe is present for older libssh versions when the socket is really closed twice.

[Ilyesbenhamouda]

For session server (src/session_server.c):
In nc_accept_ssh_session() (called by nc_accept()), the SSH socket is created by us but not passed via options.
The socket is then closed twice in nc_session_free_transport(): once by libssh in ssh_disconnect() and again by us.

ssh_get_fd() returns the session socket FD which may be owned by libssh or by the application (passed through options).
However, we must distinguish between a socket owned by us (passed through options) and a socket owned by libssh for cleanup.
Currently, there is no API in libssh that can retrieve the socket FD passed via options (i.e. using ssh_options_get() and SSH_OPTIONS_FD).
Therefore, opt_sock is introduced to store the socket FD passed through options.
This ensures correct cleanup by closing only the socket FD we own.

However, since all sockets are currently created by us, the fix can be:

diff --git a/src/session.c b/src/session.c
index 2d1b042..c9e7ffe 100644
--- a/src/session.c
+++ b/src/session.c
@@ -816,12 +816,18 @@ nc_session_free_transport(struct nc_session *session, int *multisession)
                     free(siter);
                 } while (session->ti.libssh.next != session);
             }
-            /* remember sock so we can close it */
-            sock = ssh_get_fd(session->ti.libssh.session);
             if (connected) {
-                /* does not close sock */
+                /* remember sock so we can close it */
+                sock = ssh_get_fd(session->ti.libssh.session);
+                /* resets ssh sock but only closes it if not passed through
+                 * options since libssh 0.10
+                 */
                 ssh_disconnect(session->ti.libssh.session);
+#if (LIBSSH_VERSION_MAJOR == 0 && LIBSSH_VERSION_MINOR < 10)
+                sock = -1;
+#endif
             }
+            /* closes ssh sock if it is still open */
             ssh_free(session->ti.libssh.session);
         } else {
             /* remove the session from the list */
diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c
index 3652707..1de4b61 100644
--- a/src/session_server_ssh.c
+++ b/src/session_server_ssh.c
@@ -2016,6 +2016,11 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt
         goto cleanup;
     }
 
+    /**
+     * Use SSH_OPTIONS_FD for our socket so libssh does not close it
+     * in ssh_disconnect() (libssh >= 0.10)
+     */
+    ssh_options_set(session->ti.libssh.session, SSH_OPTIONS_FD, &sock);
     /* accept new connection on the bind */
     if (ssh_bind_accept_fd(sbind, session->ti.libssh.session, sock) == SSH_ERROR) {
         ERR(session, "SSH failed to accept a new connection (%s).", ssh_get_error(sbind));

If you agree, I can update the patch to use this second solution.

[michalvasko]

Okay, that seems better. Just please leave empty lines before the comments and make sure sock is not closed twice in nc_accept_ssh_session() in case ssh_bind_accept_fd() fails.