Remove usage of deprecated libssh functions

3rd-party/libssh/CMakeLists.txt

@@ -19,8 +19,9 @@ set(WITH_SFTP TRUE)
 
 if(MSVC)
   add_definitions(-Dssize_t=SSIZE_T)
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /wd4996")
 else()
-  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-implicit-function-declaration -Wno-incompatible-pointer-types -Wno-int-conversion")
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-implicit-function-declaration -Wno-incompatible-pointer-types -Wno-int-conversion -Wno-deprecated-declarations")
 endif()
 
 set(LIBSSH_INCLUDE_DIRS
@@ -38,6 +39,9 @@ if(LINUX)
     set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 else()
     # This applies to other platforms (e.g., Windows, macOS)
+    if(MSVC)
+      set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
+    endif()
     set(BUILD_SHARED_LIBS ON)
 endif()
 

CMakeLists.txt

@@ -310,7 +310,7 @@ if(MSVC)
   set(MULTIPASS_BACKENDS hyperv virtualbox)
   set(MULTIPASS_PLATFORM windows)
 else()
-  add_compile_options(-Werror -Wall -pedantic -fPIC -Wno-error=deprecated-declarations)
+  add_compile_options(-Werror -Wall -pedantic -fPIC)
 
   if(APPLE)
     add_definitions(-DMULTIPASS_PLATFORM_APPLE)

include/multipass/ssh/ssh_client.h

@@ -51,6 +51,7 @@ class SSHClient
 private:
     void handle_ssh_events();
     int exec_string(const std::string& cmd_line);
+    int ssh_channel_get_exit_status(ssh_channel channel);
 
     SSHSessionUPtr ssh_session;
     ChannelUPtr channel;

src/ssh/ssh_client.cpp

@@ -15,6 +15,7 @@
  *
  */
 
+#include <multipass/logging/log.h>
 #include <multipass/ssh/ssh_client.h>
 #include <multipass/ssh/throw_on_error.h>
 #include <multipass/utils.h>
@@ -27,6 +28,7 @@
 #endif
 
 namespace mp = multipass;
+namespace mpl = multipass::logging;
 
 namespace
 {
@@ -153,6 +155,31 @@ int mp::SSHClient::exec_string(const std::string& cmd_line)
                             cmd_line.c_str());
 
     handle_ssh_events();
+    return this->ssh_channel_get_exit_status(channel.get());
+}
+
+int mp::SSHClient::ssh_channel_get_exit_status(ssh_channel channel)
+{
+
+    uint32_t exit_status = static_cast<uint32_t>(-1);
+    char* exit_signal_status = nullptr;
+    int core_dumped = 0;
+
+    int result{
+        ssh_channel_get_exit_state(channel, &exit_status, &exit_signal_status, &core_dumped)};
 
-    return ssh_channel_get_exit_status(channel.get());
+    if (result != SSH_OK)
+        // If SSH_ERROR or SSH_AGAIN, the string is never allocated
+        return SSH_ERROR;
+
+    if (exit_signal_status != nullptr)
+    {
+        mpl::error("ssh client",
+                   "Process terminated by signal: {}{}\n",
+                   exit_signal_status,
+                   core_dumped ? " (core dumped)" : "");
+
+        ssh_string_free_char(exit_signal_status);
+    }
+    return static_cast<int>(exit_status);
 }

src/sshfs_mount/sftp_server.cpp

@@ -19,12 +19,16 @@
 
 #include <multipass/cli/client_platform.h>
 #include <multipass/exceptions/exitless_sshprocess_exceptions.h>
+#include <multipass/exceptions/ssh_exception.h>
 #include <multipass/logging/log.h>
 #include <multipass/platform.h>
 #include <multipass/ssh/ssh_session.h>
 #include <multipass/ssh/throw_on_error.h>
+
 #include <multipass/utils.h>
 
+#include <libssh/sftp_priv.h>
+
 #include <QDir>
 #include <QFile>
 
@@ -55,11 +59,33 @@ enum Permissions
 auto make_sftp_session(ssh_session session, ssh_channel channel)
 {
     mp::SftpServer::SftpSessionUptr sftp_server_session{sftp_server_new(session, channel),
-                                                        sftp_free};
-    mp::SSH::throw_on_error(sftp_server_session,
-                            session,
-                            "[sftp] server init failed",
-                            sftp_server_init);
+                                                        sftp_server_free};
+    // The function sftp_server_init was expanded here to avoid deprecation warnings.
+    // TODO: move to callback-based sftp implementations.
+    // https://github.com/canonical/multipass/issues/4445
+
+    /* handles setting the sftp->client_version */
+    sftp_client_message msg{sftp_get_client_message(sftp_server_session.get())};
+    if (msg == nullptr)
+    {
+        throw mp::SSHException("[sftp] server init failed: 'Null client message'");
+    }
+
+    if (msg->type != SSH_FXP_INIT)
+    {
+        throw mp::SSHException(fmt::format(
+            "[sftp] server init failed: 'FATAL: Packet read of type {} instead of SSH_FXP_INIT'",
+            msg->type));
+    }
+
+    // Optional: Log the SSH_FXP_INIT reception like libssh does with SSH_LOG but with mp::log
+
+    if (sftp_reply_version(msg) != SSH_OK)
+    {
+        throw mp::SSHException(
+            "[sftp] server init failed: 'FATAL: Failed to process the SSH_FXP_INIT message'");
+    }
+
     return sftp_server_session;
 }
 

src/sshfs_mount/sftp_server.h

@@ -53,7 +53,7 @@ class SftpServer
     void stop();
 
     using SSHSessionUptr = std::unique_ptr<ssh_session_struct, decltype(ssh_free)*>;
-    using SftpSessionUptr = std::unique_ptr<sftp_session_struct, decltype(sftp_free)*>;
+    using SftpSessionUptr = std::unique_ptr<sftp_session_struct, decltype(sftp_server_free)*>;
     using SSHFSProcUptr = std::unique_ptr<SSHProcess>;
 
 private:

tests/c_mock_defines.cmake

@@ -43,19 +43,19 @@ add_c_mocks(
   ssh_channel_request_pty
   ssh_channel_change_pty_size
   ssh_channel_read_timeout
-  ssh_channel_get_exit_status
+  ssh_channel_get_exit_state
   ssh_event_dopoll
   ssh_add_channel_callbacks
   sftp_server_new
-  sftp_free
-  sftp_server_init
+  sftp_server_free
   sftp_reply_status
   sftp_reply_attr
   sftp_reply_data
   sftp_reply_name
   sftp_reply_names
   sftp_reply_names_add
   sftp_reply_handle
+  sftp_reply_version
   sftp_get_client_message
   sftp_client_message_free
   sftp_client_message_get_data

tests/mock_sftpserver.cpp

@@ -19,7 +19,7 @@
 extern "C"
 {
 IMPL_MOCK_DEFAULT(2, sftp_server_new);
-IMPL_MOCK_DEFAULT(1, sftp_server_init);
+IMPL_MOCK_DEFAULT(1, sftp_server_free);
 IMPL_MOCK_DEFAULT(3, sftp_reply_status);
 IMPL_MOCK_DEFAULT(2, sftp_reply_attr);
 IMPL_MOCK_DEFAULT(3, sftp_reply_data);
@@ -34,4 +34,5 @@ IMPL_MOCK_DEFAULT(1, sftp_client_message_get_filename);
 IMPL_MOCK_DEFAULT(2, sftp_handle);
 IMPL_MOCK_DEFAULT(2, sftp_handle_alloc);
 IMPL_MOCK_DEFAULT(2, sftp_handle_remove);
+IMPL_MOCK_DEFAULT(1, sftp_reply_version);
 }

tests/mock_sftpserver.h

@@ -20,9 +20,10 @@
 #include <premock.hpp>
 
 #include <libssh/sftp.h>
+#include <libssh/sftp_priv.h>
 
 DECL_MOCK(sftp_server_new);
-DECL_MOCK(sftp_server_init);
+DECL_MOCK(sftp_server_free);
 DECL_MOCK(sftp_reply_status);
 DECL_MOCK(sftp_reply_attr);
 DECL_MOCK(sftp_reply_data);
@@ -34,6 +35,7 @@ DECL_MOCK(sftp_get_client_message);
 DECL_MOCK(sftp_client_message_free);
 DECL_MOCK(sftp_client_message_get_data);
 DECL_MOCK(sftp_client_message_get_filename);
+DECL_MOCK(sftp_reply_version);
 DECL_MOCK(sftp_handle);
 DECL_MOCK(sftp_handle_alloc);
 DECL_MOCK(sftp_handle_remove);

tests/mock_ssh.cpp

@@ -30,7 +30,7 @@ IMPL_MOCK_DEFAULT(1, ssh_channel_new);
 IMPL_MOCK_DEFAULT(1, ssh_channel_open_session);
 IMPL_MOCK_DEFAULT(2, ssh_channel_request_exec);
 IMPL_MOCK_DEFAULT(5, ssh_channel_read_timeout);
-IMPL_MOCK_DEFAULT(1, ssh_channel_get_exit_status);
+IMPL_MOCK_DEFAULT(4, ssh_channel_get_exit_state);
 IMPL_MOCK_DEFAULT(2, ssh_event_dopoll);
 IMPL_MOCK_DEFAULT(2, ssh_add_channel_callbacks);
 IMPL_MOCK_DEFAULT(1, ssh_get_error);

tests/mock_ssh.h

@@ -34,7 +34,7 @@ DECL_MOCK(ssh_channel_new);
 DECL_MOCK(ssh_channel_open_session);
 DECL_MOCK(ssh_channel_request_exec);
 DECL_MOCK(ssh_channel_read_timeout);
-DECL_MOCK(ssh_channel_get_exit_status);
+DECL_MOCK(ssh_channel_get_exit_state);
 DECL_MOCK(ssh_event_dopoll);
 DECL_MOCK(ssh_add_channel_callbacks);
 DECL_MOCK(ssh_get_error);

tests/mock_ssh_test_fixture.h

@@ -36,7 +36,7 @@ struct MockSSHTestFixture
         request_exec.returnValue(SSH_OK);
         channel_read.returnValue(0);
         is_eof.returnValue(true);
-        get_exit_status.returnValue(SSH_OK);
+        get_exit_state.returnValue(SSH_OK);
         channel_is_open.returnValue(true);
         channel_is_closed.returnValue(0);
         options_set.returnValue(SSH_OK);
@@ -49,7 +49,7 @@ struct MockSSHTestFixture
     decltype(MOCK(ssh_channel_request_exec)) request_exec{MOCK(ssh_channel_request_exec)};
     decltype(MOCK(ssh_channel_read_timeout)) channel_read{MOCK(ssh_channel_read_timeout)};
     decltype(MOCK(ssh_channel_is_eof)) is_eof{MOCK(ssh_channel_is_eof)};
-    decltype(MOCK(ssh_channel_get_exit_status)) get_exit_status{MOCK(ssh_channel_get_exit_status)};
+    decltype(MOCK(ssh_channel_get_exit_state)) get_exit_state{MOCK(ssh_channel_get_exit_state)};
     decltype(MOCK(ssh_channel_is_open)) channel_is_open{MOCK(ssh_channel_is_open)};
     decltype(MOCK(ssh_channel_is_closed)) channel_is_closed{MOCK(ssh_channel_is_closed)};
     decltype(MOCK(ssh_options_set)) options_set{MOCK(ssh_options_set)};

tests/sftp_server_test_fixture.h

@@ -29,23 +29,23 @@ namespace test
 struct SftpServerTest : public testing::Test
 {
     SftpServerTest()
-        : free_sftp{mock_sftp_free, [](sftp_session sftp) {
-                        std::free(sftp->handles);
-                        std::free(sftp);
-                    }}
+        : free_server_sftp{mock_sftp_server_free, [](sftp_session sftp) {
+                               std::free(sftp->handles);
+                               std::free(sftp);
+                           }}
     {
-        init_sftp.returnValue(SSH_OK);
         reply_status.returnValue(SSH_OK);
         get_client_msg.returnValue(nullptr);
         handle_sftp.returnValue(nullptr);
+        reply_version.returnValue(SSH_OK);
     }
 
-    decltype(MOCK(sftp_server_init)) init_sftp{MOCK(sftp_server_init)};
     decltype(MOCK(sftp_reply_status)) reply_status{MOCK(sftp_reply_status)};
     decltype(MOCK(sftp_get_client_message)) get_client_msg{MOCK(sftp_get_client_message)};
     decltype(MOCK(sftp_client_message_free)) msg_free{MOCK(sftp_client_message_free)};
     decltype(MOCK(sftp_handle)) handle_sftp{MOCK(sftp_handle)};
-    MockScope<decltype(mock_sftp_free)> free_sftp;
+    decltype(MOCK(sftp_reply_version)) reply_version{MOCK(sftp_reply_version)};
+    MockScope<decltype(mock_sftp_server_free)> free_server_sftp;
 
     MockSSHTestFixture mock_ssh_test_fixture;
 };

tests/test_cli_client.cpp

@@ -1777,11 +1777,14 @@ TEST_P(SSHClientReturnTest, execCmdWithoutDirWorks)
 {
     const int failure_code{GetParam()};
 
-    REPLACE(ssh_channel_get_exit_status, [&failure_code](auto) { return failure_code; });
-
     std::string instance_name{"instance"};
     mp::SSHInfoReply response = make_fake_ssh_info_response(instance_name);
 
+    REPLACE(ssh_channel_get_exit_state,
+            [failure_code](ssh_channel_struct*, unsigned int* val, char**, int*) {
+                *val = failure_code;
+                return failure_code == -1 ? -1 : SSH_OK;
+            });
     EXPECT_CALL(mock_daemon, ssh_info(_, _))
         .WillOnce(
             [&response](grpc::ServerContext* context,
@@ -1799,11 +1802,14 @@ TEST_P(SSHClientReturnTest, execCmdWithDirWorks)
 {
     const int failure_code{GetParam()};
 
-    REPLACE(ssh_channel_get_exit_status, [&failure_code](auto) { return failure_code; });
-
     std::string instance_name{"instance"};
     mp::SSHInfoReply response = make_fake_ssh_info_response(instance_name);
 
+    REPLACE(ssh_channel_get_exit_state,
+            [failure_code](ssh_channel_struct*, unsigned int* val, char**, int*) {
+                *val = failure_code;
+                return failure_code == -1 ? -1 : SSH_OK;
+            });
     EXPECT_CALL(mock_daemon, ssh_info(_, _))
         .WillOnce(
             [&response](grpc::ServerContext* context,
@@ -1824,6 +1830,10 @@ TEST_F(Client, execCmdWithDirPrependsCd)
     std::string dir{"/home/ubuntu/"};
     std::string cmd{"pwd"};
 
+    REPLACE(ssh_channel_get_exit_state, [](ssh_channel_struct*, unsigned int* val, char**, int*) {
+        *val = 0;
+        return SSH_OK;
+    });
     REPLACE(ssh_channel_request_exec, ([&dir, &cmd](ssh_channel, const char* raw_cmd) {
                 EXPECT_THAT(raw_cmd, StartsWith("cd " + dir));
                 EXPECT_THAT(raw_cmd, HasSubstr("&&"));
@@ -1856,6 +1866,10 @@ TEST_F(Client, execCmdWithDirAndSudoUsesSh)
     for (size_t i = 1; i < cmds.size(); ++i)
         cmds_string += " " + cmds[i];
 
+    REPLACE(ssh_channel_get_exit_state, [](ssh_channel_struct*, unsigned int* val, char**, int*) {
+        *val = 0;
+        return SSH_OK;
+    });
     REPLACE(ssh_channel_request_exec, ([&dir, &cmds_string](ssh_channel, const char* raw_cmd) {
                 // The test expects this exact command format
                 // The issue is that when using sudo -u user, the AppArmor context is not preserved
@@ -4625,6 +4639,10 @@ TEST_F(ClientAlias, execAliasRewritesMountedDir)
 
     populate_db_file(AliasesVector{{alias_name, {instance_name, cmd, "map"}}});
 
+    REPLACE(ssh_channel_get_exit_state, [](ssh_channel_struct*, unsigned int* val, char**, int*) {
+        *val = 0;
+        return SSH_OK;
+    });
     REPLACE(ssh_channel_request_exec, ([&target_dir, &cmd](ssh_channel, const char* raw_cmd) {
                 EXPECT_THAT(raw_cmd, StartsWith("cd " + target_dir + "/"));
                 EXPECT_THAT(raw_cmd, HasSubstr("&&"));
@@ -4673,6 +4691,10 @@ TEST_P(NotDirRewriteTestsuite, execAliasDoesNotRewriteMountedDir)
     populate_db_file(
         AliasesVector{{alias_name, {instance_name, cmd, map_dir ? "map" : "default"}}});
 
+    REPLACE(ssh_channel_get_exit_state, [](ssh_channel_struct*, unsigned int* val, char**, int*) {
+        *val = 0;
+        return SSH_OK;
+    });
     REPLACE(ssh_channel_request_exec, ([&cmd](ssh_channel, const char* raw_cmd) {
                 EXPECT_THAT(raw_cmd, Not(StartsWith("cd ")));
                 EXPECT_THAT(raw_cmd, Not(HasSubstr("&&")));