Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update libssh to 0.11.1 #3735

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 3rd-party/libssh/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ file(STRINGS libssh/CMakeLists.txt libssh_VERSION REGEX "^project\\(libssh")
file(STRINGS libssh/CMakeLists.txt LIBRARY_VERSION REGEX "^set\\(LIBRARY_VERSION")
file(STRINGS libssh/CMakeLists.txt LIBRARY_SOVERSION REGEX "^set\\(LIBRARY_SOVERSION")

string(REGEX MATCH "^project\\(libssh VERSION ([0-9]+)\\.([0-9]+)\\.([0-9]+) LANGUAGES C\\)$"
string(REGEX MATCH "^project\\(libssh VERSION ([0-9]+)\\.([0-9]+)\\.([0-9]+) LANGUAGES C CXX\\)$"
libssh_VERSION "${libssh_VERSION}")
if (NOT libssh_VERSION)
message(FATAL_ERROR "unable to find libssh project version")
Expand Down
2 changes: 1 addition & 1 deletion 3rd-party/libssh/libssh
2 changes: 1 addition & 1 deletion 3rd-party/submodule_info.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Version: 1.52.1 (+[our patches](https://github.com/CanonicalLtd/grpc/compare/v1.
<https://github.com/grpc/grpc/releases>

### libssh
Version: 0.10.6 (+[our patches](https://github.com/canonical/libssh/compare/libssh-0.10.6...multipass)) |
Version: 0.11.1 (+[our patches](https://github.com/canonical/libssh/compare/libssh-0.11.1...multipass)) |
<https://github.com/canonical/libssh.git> |
<https://www.libssh.org/>

Expand Down
6 changes: 5 additions & 1 deletion include/multipass/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ std::unique_ptr<Process> make_sshfs_server_process(const SSHFSServerConfig& conf
std::unique_ptr<Process> make_process(std::unique_ptr<ProcessSpec>&& process_spec);
int symlink_attr_from(const char* path, sftp_attributes_struct* attr);

std::function<int()> make_quit_watchdog(); // call while single-threaded; call result later, in dedicated thread
// Creates a function that will wait for signals or until the passed function returns false.
// The passed function is checked every `period` milliseconds.
// If a signal is received the optional contains it, otherwise the optional is empty.
std::function<std::optional<int>(const std::function<bool()>&)> make_quit_watchdog(
Sploder12 marked this conversation as resolved.
Show resolved Hide resolved
ricab marked this conversation as resolved.
Show resolved Hide resolved
const std::chrono::milliseconds& period); // call while single-threaded; call result later, in dedicated thread

std::string reinterpret_interface_id(const std::string& ux_id); // give platforms a chance to reinterpret network IDs

Expand Down
21 changes: 17 additions & 4 deletions include/multipass/platform_unix.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,25 @@

#include <signal.h>

namespace multipass
#include "singleton.h"

#define MP_POSIX_SIGNAL multipass::platform::SignalWrapper::instance()

namespace multipass::platform
{
namespace platform

class SignalWrapper : public Singleton<SignalWrapper>
{
public:
SignalWrapper(const PrivatePass&) noexcept;

virtual int mask_signals(int how, const sigset_t* sigset, sigset_t* old_set = nullptr) const;
virtual int send(pthread_t target, int signal) const;
virtual int wait(const sigset_t& sigset, int& got) const;
};

sigset_t make_sigset(const std::vector<int>& sigs);
sigset_t make_and_block_signals(const std::vector<int>& sigs);
} // namespace platform
}

} // namespace multipass::platform
#endif // MULTIPASS_PLATFORM_UNIX_H
6 changes: 1 addition & 5 deletions include/multipass/ssh/ssh_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ class SSHKeyProvider;
class SSHSession
{
public:
SSHSession(const std::string& host,
int port,
const std::string& ssh_username,
const SSHKeyProvider& key_provider,
const std::chrono::milliseconds timeout = std::chrono::seconds(20));
SSHSession(const std::string& host, int port, const std::string& ssh_username, const SSHKeyProvider& key_provider);

// just being explicit (unique_ptr member already caused these to be deleted)
SSHSession(const SSHSession&) = delete;
Expand Down
5 changes: 3 additions & 2 deletions src/platform/console/unix_console.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,14 @@

if (term->is_live())
{
setup_console();

const char* term_type = std::getenv("TERM");
term_type = (term_type == nullptr) ? "xterm" : term_type;

update_local_pty_size(term->cout_fd());
ssh_channel_request_pty_size(channel, term_type, local_pty_size.columns, local_pty_size.rows);

// set stdin to Raw Mode after libssh inherits sane settings from stdin.
georgeliao marked this conversation as resolved.
Show resolved Hide resolved
setup_console();

Check warning on line 81 in src/platform/console/unix_console.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/console/unix_console.cpp#L81

Added line #L81 was not covered by tests
}
}

Expand Down
55 changes: 49 additions & 6 deletions src/platform/platform_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <multipass/format.h>
#include <multipass/platform.h>
#include <multipass/platform_unix.h>
#include <multipass/timer.h>
#include <multipass/utils.h>

#include <grp.h>
Expand Down Expand Up @@ -157,6 +158,25 @@
return 0;
}

mp::platform::SignalWrapper::SignalWrapper(const PrivatePass& pass) noexcept : Singleton(pass)
{
}

int mp::platform::SignalWrapper::mask_signals(int how, const sigset_t* sigset, sigset_t* old_set) const

Check warning on line 165 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L165

Added line #L165 was not covered by tests
{
return pthread_sigmask(how, sigset, old_set);

Check warning on line 167 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L167

Added line #L167 was not covered by tests
}

int mp::platform::SignalWrapper::send(pthread_t target, int signal) const

Check warning on line 170 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L170

Added line #L170 was not covered by tests
{
return pthread_kill(target, signal);

Check warning on line 172 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L172

Added line #L172 was not covered by tests
}

int mp::platform::SignalWrapper::wait(const sigset_t& sigset, int& got) const

Check warning on line 175 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L175

Added line #L175 was not covered by tests
{
return sigwait(std::addressof(sigset), std::addressof(got));

Check warning on line 177 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L177

Added line #L177 was not covered by tests
}

sigset_t mp::platform::make_sigset(const std::vector<int>& sigs)
{
sigset_t sigset;
Expand All @@ -171,15 +191,38 @@
sigset_t mp::platform::make_and_block_signals(const std::vector<int>& sigs)
{
auto sigset{make_sigset(sigs)};
pthread_sigmask(SIG_BLOCK, &sigset, nullptr);
MP_POSIX_SIGNAL.mask_signals(SIG_BLOCK, &sigset, nullptr);
return sigset;
}

std::function<int()> mp::platform::make_quit_watchdog()
template <class Rep, class Period>
timespec make_timespec(std::chrono::duration<Rep, Period> duration)
{
const auto seconds = std::chrono::duration_cast<std::chrono::seconds>(duration);
return timespec{seconds.count(), std::chrono::duration_cast<std::chrono::nanoseconds>(duration - seconds).count()};
}
Sploder12 marked this conversation as resolved.
Show resolved Hide resolved

std::function<std::optional<int>(const std::function<bool()>&)> mp::platform::make_quit_watchdog(
const std::chrono::milliseconds& period)
{
return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP})]() {
int sig = -1;
sigwait(&sigset, &sig);
return sig;
return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}),
period](const std::function<bool()>& condition) -> std::optional<int> {
// create a timer to periodically send SIGUSR2
utils::Timer signal_generator{period, [signalee = pthread_self()] { MP_POSIX_SIGNAL.send(signalee, SIGUSR2); }};

// wait on signals and condition
int latest_signal = SIGUSR2;
while (latest_signal == SIGUSR2 && condition())
{
signal_generator.start();

// can't use sigtimedwait since macOS doesn't support it
MP_POSIX_SIGNAL.wait(sigset, latest_signal);
}

signal_generator.stop();

// if `latest_signal` is SIGUSR2 then we know `condition()` is false
return latest_signal == SIGUSR2 ? std::nullopt : std::make_optional(latest_signal);
};
}
5 changes: 2 additions & 3 deletions src/ssh/ssh_session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,13 @@ namespace mpl = multipass::logging;
mp::SSHSession::SSHSession(const std::string& host,
int port,
const std::string& username,
const SSHKeyProvider& key_provider,
const std::chrono::milliseconds timeout)
const SSHKeyProvider& key_provider)
: session{ssh_new(), ssh_free}, mut{}
{
if (session == nullptr)
throw mp::SSHException("could not allocate ssh session");

const long timeout_secs = std::chrono::duration_cast<std::chrono::seconds>(timeout).count();
const long timeout_secs = std::numeric_limits<long>::max();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About this change, I think the timeout_secs is used to set the SSH_OPTIONS_TIMEOUT, and this option is the timeout value for ssh session connection establishment wait time (not entire sure about this, hard to find a clear documentation as well). Therefore, having a reasonable value (like the original 20 seconds) seems to makes sense here.

Besides, with your watchdog fix ( which is essentially checking the sftp_threading running together with other SIGQUIT, SIGTERM, SIGHUP signals), the program already behaves correctly. So I am doubting whether we still make the time out change here. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this a keep-alive timeout? A better var name might help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The program will not behave correctly with a reasonable timeout unfortunately. After the timeout expires, the sshfs_server process will exit, the watchdog fix makes it so that it doesn't hang. It behaves like a keep-alive timeout, but libssh has no keep-alive messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so it looks like a keep-alive timeout as opposed to ssh session connection establishment wait time, interesting. It is a weird option offered to users though.

Copy link
Contributor

@georgeliao georgeliao Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a wondering here though, the SSH_OPTIONS_TIMEOUT is the a ssh option. If it were a keep-alive timeout, should it be the timeout of ssh session alive? If so, should the multipass shell hang without mount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multipass shell doesn't hang since it's setup to automatically reconnect. You can see this behavior in #3810 where the SSH session is restarting every 20 seconds. Also with normal use it's much more likely there is some SSH traffic within the 20 seconds compared to SFTP which is likely to not get any traffic for long periods of time.

const int nodelay{1};
auto ssh_dir = QDir(MP_STDPATHS.writableLocation(StandardPaths::AppConfigLocation)).filePath("ssh").toStdString();

Expand Down
19 changes: 16 additions & 3 deletions src/sshfs_mount/sshfs_mount.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,21 +151,29 @@

} // namespace

mp::SshfsMount::SshfsMount(SSHSession&& session, const std::string& source, const std::string& target,
const mp::id_mappings& gid_mappings, const mp::id_mappings& uid_mappings)
mp::SshfsMount::SshfsMount(SSHSession&& session,
const std::string& source,
const std::string& target,
const mp::id_mappings& gid_mappings,
const mp::id_mappings& uid_mappings)
: sftp_server{make_sftp_server(std::move(session), source, target, gid_mappings, uid_mappings)},
sftp_thread{[this]() {
sftp_thread{[this] {
state.store(State::Running, std::memory_order_release);

mp::top_catch_all(category, [this] {
std::cout << "Connected" << std::endl;
sftp_server->run();
std::cout << "Stopped" << std::endl;
});

state.store(State::Stopped, std::memory_order_release);
}}
{
}

mp::SshfsMount::~SshfsMount()
{
state.store(State::Stopped, std::memory_order_release);
stop();
}

Expand All @@ -175,3 +183,8 @@
if (sftp_thread.joinable())
sftp_thread.join();
}

bool mp::SshfsMount::alive() const

Check warning on line 187 in src/sshfs_mount/sshfs_mount.cpp

View check run for this annotation

Codecov / codecov/patch

src/sshfs_mount/sshfs_mount.cpp#L187

Added line #L187 was not covered by tests
{
return state.load(std::memory_order_acquire) != State::Stopped;

Check warning on line 189 in src/sshfs_mount/sshfs_mount.cpp

View check run for this annotation

Codecov / codecov/patch

src/sshfs_mount/sshfs_mount.cpp#L189

Added line #L189 was not covered by tests
}
10 changes: 10 additions & 0 deletions src/sshfs_mount/sshfs_mount.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,17 @@ class SshfsMount

void stop();

[[nodiscard]] bool alive() const;

private:
enum class State
{
Unstarted,
Running,
Stopped
};

std::atomic<State> state{State::Unstarted};
// sftp_server Doesn't need to be a pointer, but done for now to avoid bringing sftp.h
// which has an error with -pedantic.
std::unique_ptr<SftpServer> sftp_server;
Expand Down
13 changes: 9 additions & 4 deletions src/sshfs_mount/sshfs_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,17 +105,22 @@ int main(int argc, char* argv[])

try
{
auto watchdog = mpp::make_quit_watchdog(); // called while there is only one thread
auto watchdog =
mpp::make_quit_watchdog(std::chrono::milliseconds{500}); // called while there is only one thread

mp::SSHSession session{host, port, username, mp::SSHClientKeyProvider{priv_key_blob}};
mp::SshfsMount sshfs_mount(std::move(session), source_path, target_path, gid_mappings, uid_mappings);

// ssh lives on its own thread, use this thread to listen for quit signal
if (int sig = watchdog())
cout << "Received signal " << sig << ". Stopping" << endl;
auto sig = watchdog([&sshfs_mount] { return sshfs_mount.alive(); });

if (sig.has_value())
cout << "Received signal " << *sig << ". Stopping" << endl;
else
cerr << "SFTP server thread stopped unexpectedly." << endl;

sshfs_mount.stop();
exit(0);
exit(sig.has_value() ? EXIT_SUCCESS : EXIT_FAILURE);
}
catch (const mp::SSHFSMissingError&)
{
Expand Down
42 changes: 42 additions & 0 deletions tests/unix/mock_signal_wrapper.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright (C) Canonical, Ltd.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; version 3.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#ifndef MULTIPASS_MOCK_SIGNAL_WRAPPER_H
#define MULTIPASS_MOCK_SIGNAL_WRAPPER_H

#include "../common.h"
#include "../mock_singleton_helpers.h"

#include <multipass/platform_unix.h>

namespace multipass::test
{

class MockSignalWrapper : public platform::SignalWrapper
{
public:
using SignalWrapper::SignalWrapper;

MOCK_METHOD(int, mask_signals, (int, const sigset_t*, sigset_t*), (const, override));
MOCK_METHOD(int, send, (pthread_t, int), (const, override));
MOCK_METHOD(int, wait, (const sigset_t&, int&), (const, override));

MP_MOCK_SINGLETON_BOILERPLATE(MockSignalWrapper, platform::SignalWrapper);
};

} // namespace multipass::test

#endif // MULTIPASS_MOCK_SIGNAL_WRAPPER_H
Loading
Loading