From 384a8da50bfa7b0e52f88aff67a766b390667f7b Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Fri, 7 May 2021 10:01:34 +0200 Subject: [PATCH] ssh_terminal: use pthread_mutex to avoid sporadic deadlocks between EP thread and the server loop pthread. Issue #4095 --- repos/gems/src/server/ssh_terminal/login.h | 6 +-- .../src/server/ssh_terminal/root_component.h | 14 +++--- repos/gems/src/server/ssh_terminal/server.cc | 46 ++++++++----------- repos/gems/src/server/ssh_terminal/server.h | 12 +---- .../src/server/ssh_terminal/ssh_callbacks.cc | 9 ++-- repos/gems/src/server/ssh_terminal/terminal.h | 12 +++-- repos/gems/src/server/ssh_terminal/util.h | 45 ++++++++++++++++-- 7 files changed, 86 insertions(+), 58 deletions(-) diff --git a/repos/gems/src/server/ssh_terminal/login.h b/repos/gems/src/server/ssh_terminal/login.h index c4b946df91..475fd11d36 100644 --- a/repos/gems/src/server/ssh_terminal/login.h +++ b/repos/gems/src/server/ssh_terminal/login.h @@ -108,8 +108,8 @@ struct Ssh::Login : Genode::Registry::Element struct Ssh::Login_registry : Genode::Registry { - Genode::Allocator &_alloc; - Genode::Mutex _mutex { }; + Genode::Allocator &_alloc; + Util::Pthread_mutex _mutex { }; /** * Import one login from node @@ -155,7 +155,7 @@ struct Ssh::Login_registry : Genode::Registry /** * Return registry mutex */ - Genode::Mutex &mutex() { return _mutex; } + Util::Pthread_mutex &mutex() { return _mutex; } /** * Import all login information from config diff --git a/repos/gems/src/server/ssh_terminal/root_component.h b/repos/gems/src/server/ssh_terminal/root_component.h index fce00e5cef..2c19bd90da 100644 --- a/repos/gems/src/server/ssh_terminal/root_component.h +++ b/repos/gems/src/server/ssh_terminal/root_component.h @@ -55,12 +55,14 @@ class Terminal::Root_component : public Genode::Root_componentsend(sess.channel); } - catch (...) { } - }); + try { sess.terminal->send(sess.channel); } + catch (...) { } }; _sessions.for_each(invalidate_terminal); @@ -393,7 +393,7 @@ void Ssh::Server::detach_terminal(Ssh::Terminal &conn) void Ssh::Server::update_config(Genode::Xml_node const &config) { - Genode::Mutex::Guard guard(_terminals.mutex()); + Util::Pthread_mutex::Guard guard(_terminals.mutex()); _parse_config(config); ssh_bind_options_set(_ssh_bind, SSH_BIND_OPTIONS_LOG_VERBOSITY, &_log_level); @@ -425,7 +425,7 @@ Ssh::Session *Ssh::Server::lookup_session(ssh_session s) bool Ssh::Server::request_terminal(Session &session, const char* command) { - Genode::Mutex::Guard guard(_logins.mutex()); + Util::Pthread_mutex::Guard guard(_logins.mutex()); Login const *l = _logins.lookup(session.user().string()); if (!l || !l->request_terminal) { return false; @@ -505,7 +505,7 @@ bool Ssh::Server::auth_password(ssh_session s, char const *u, char const *pass) * Even if there is no valid login for the user, let * the client try anyway and check multi login afterwards. */ - Genode::Mutex::Guard guard(_logins.mutex()); + Util::Pthread_mutex::Guard guard(_logins.mutex()); Login const *l = _logins.lookup(u); if (l && l->user == u && l->password == pass) { if (_allow_multi_login(s, *l)) { @@ -566,7 +566,7 @@ bool Ssh::Server::auth_pubkey(ssh_session s, char const *u, * matches allow authentication to proceed. */ if (signature_state == SSH_PUBLICKEY_STATE_VALID) { - Genode::Mutex::Guard guard(_logins.mutex()); + Util::Pthread_mutex::Guard guard(_logins.mutex()); Login const *l = _logins.lookup(u); if (l && !ssh_key_cmp(pubkey, l->pub_key, SSH_KEY_CMP_PUBLIC)) { @@ -594,7 +594,7 @@ void Ssh::Server::loop() } { - Genode::Mutex::Guard guard(_terminals.mutex()); + Util::Pthread_mutex::Guard guard(_terminals.mutex()); /* first remove all stale sessions */ auto cleanup = [&] (Session &s) { @@ -644,19 +644,13 @@ void Ssh::Server::loop() void Ssh::Server::_wake_loop() { /* wake the event loop up */ - Libc::with_libc([&] { - char c = 1; - ::write(_server_fds[1], &c, sizeof(c)); - }); + char c = 1; + ::write(_server_fds[1], &c, sizeof(c)); } static int write_avail_cb(socket_t fd, int revents, void *userdata) { - int n = 0; - Libc::with_libc([&] { - char c; - n = ::read(fd, &c, sizeof(char)); - }); - return n; + char c; + return ::read(fd, &c, sizeof(char)); } diff --git a/repos/gems/src/server/ssh_terminal/server.h b/repos/gems/src/server/ssh_terminal/server.h index 76df0e3551..f9630793fc 100644 --- a/repos/gems/src/server/ssh_terminal/server.h +++ b/repos/gems/src/server/ssh_terminal/server.h @@ -61,14 +61,6 @@ struct Ssh::Session : Genode::Registry::Element Ssh::Terminal *terminal { nullptr }; bool terminal_detached { false }; - Genode::Mutex _access_mutex { }; - Genode::Mutex &mutex_terminal() - { - return _access_mutex; - } - - bool spawn_terminal { false }; - Session(Genode::Registry ®, ssh_session s, ssh_channel_callbacks ccb, @@ -111,8 +103,8 @@ struct Ssh::Terminal_session : Genode::Registry::Element struct Ssh::Terminal_registry : Genode::Registry { - Genode::Mutex _mutex { }; - Genode::Mutex &mutex() { return _mutex; } + Util::Pthread_mutex _mutex { }; + Util::Pthread_mutex &mutex() { return _mutex; } }; diff --git a/repos/gems/src/server/ssh_terminal/ssh_callbacks.cc b/repos/gems/src/server/ssh_terminal/ssh_callbacks.cc index 3e5e2198bb..e9bf6075b5 100644 --- a/repos/gems/src/server/ssh_terminal/ssh_callbacks.cc +++ b/repos/gems/src/server/ssh_terminal/ssh_callbacks.cc @@ -36,7 +36,6 @@ int channel_data_cb(ssh_session session, ssh_channel channel, void *userdata) { using Genode::error; - using Genode::Mutex; if (len == 0) { return 0; @@ -59,10 +58,10 @@ int channel_data_cb(ssh_session session, ssh_channel channel, return SSH_ERROR; } - Ssh::Terminal &conn { *p->terminal }; - Mutex::Guard guard { conn.read_buf.mutex() }; - char const *src { reinterpret_cast(data) }; - size_t num_bytes { 0 }; + Ssh::Terminal &conn { *p->terminal }; + Util::Pthread_mutex::Guard guard { conn.read_buf.mutex() }; + char const *src { reinterpret_cast(data) }; + size_t num_bytes { 0 }; while ((conn.read_buf.write_avail() > 0) && (num_bytes < len)) { diff --git a/repos/gems/src/server/ssh_terminal/terminal.h b/repos/gems/src/server/ssh_terminal/terminal.h index ff7f2d5f94..5b78629dd9 100644 --- a/repos/gems/src/server/ssh_terminal/terminal.h +++ b/repos/gems/src/server/ssh_terminal/terminal.h @@ -209,7 +209,7 @@ class Ssh::Terminal */ size_t read(char *dst, size_t dst_len) { - Mutex::Guard guard(read_buf.mutex()); + Util::Pthread_mutex::Guard guard(read_buf.mutex()); size_t const num_bytes = min(dst_len, read_buf.read_avail()); Genode::memcpy(dst, read_buf.content(), num_bytes); @@ -268,8 +268,14 @@ class Ssh::Terminal */ bool read_buffer_empty() { - Mutex::Guard guard(read_buf.mutex()); - return !read_buf.read_avail(); + bool empty = true; + + Libc::with_libc([&] { + Util::Pthread_mutex::Guard guard(read_buf.mutex()); + empty = !read_buf.read_avail(); + }); + + return empty; } }; diff --git a/repos/gems/src/server/ssh_terminal/util.h b/repos/gems/src/server/ssh_terminal/util.h index 9d692a6c6b..84be040f34 100644 --- a/repos/gems/src/server/ssh_terminal/util.h +++ b/repos/gems/src/server/ssh_terminal/util.h @@ -21,6 +21,7 @@ #include /* libc includes */ +#include #include #include @@ -36,16 +37,50 @@ namespace Util * get the current time from the libc backend. */ char const *get_time(); + + struct Pthread_mutex; } +struct Util::Pthread_mutex +{ + public: + + class Guard + { + private: + + Pthread_mutex &_mutex; + + public: + + explicit Guard(Pthread_mutex &mutex) : _mutex(mutex) { _mutex.lock(); } + + ~Guard() { _mutex.unlock(); } + }; + + private: + + pthread_mutex_t _mutex; + + public: + + Pthread_mutex() { pthread_mutex_init(&_mutex, nullptr); } + + ~Pthread_mutex() { pthread_mutex_destroy(&_mutex); } + + void lock() { pthread_mutex_lock(&_mutex); } + void unlock() { pthread_mutex_unlock(&_mutex); } +}; + + template struct Util::Buffer { - Genode::Mutex _mutex { }; - char _data[C] { }; - size_t _head { 0 }; - size_t _tail { 0 }; + Util::Pthread_mutex _mutex { }; + char _data[C] { }; + size_t _head { 0 }; + size_t _tail { 0 }; size_t read_avail() const { return _head > _tail ? _head - _tail : 0; } size_t write_avail() const { return _head <= C ? C - _head : 0; } @@ -55,7 +90,7 @@ struct Util::Buffer void consume(size_t n) { _tail += n; } void reset() { _head = _tail = 0; } - Genode::Mutex &mutex() { return _mutex; } + Util::Pthread_mutex &mutex() { return _mutex; } }; #endif /* _SSH_TERMINAL_UTIL_H_ */