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

@@ -108,8 +108,8 @@ struct Ssh::Login : Genode::Registry<Ssh::Login>::Element
 
 struct Ssh::Login_registry : Genode::Registry<Ssh::Login>
 {
-	Genode::Allocator &_alloc;
+	Genode::Allocator   &_alloc;
-	Genode::Mutex      _mutex { };
+	Util::Pthread_mutex  _mutex { };
 
 	/**
 	 * Import one login from node
@@ -155,7 +155,7 @@ struct Ssh::Login_registry : Genode::Registry<Ssh::Login>
 	/**
 	 * Return registry mutex 
 	 */
-	Genode::Mutex &mutex() { return _mutex; }
+	Util::Pthread_mutex &mutex() { return _mutex; }
 
 	/**
 	 * Import all login information from config

repos/gems/src/server/ssh_terminal/root_component.h

@@ -55,12 +55,14 @@ class Terminal::Root_component : public Genode::Root_component<Session_component
 			_config_rom.update();
 			if (!_config_rom.valid()) { return; }
 
-			{
+			Libc::with_libc([&] () {
-				Genode::Mutex::Guard guard(_logins.mutex());
+				{
-				_logins.import(_config_rom.xml());
+					Util::Pthread_mutex::Guard guard(_logins.mutex());
-			}
+					_logins.import(_config_rom.xml());
+				}
 
-			_server.update_config(_config_rom.xml());
+				_server.update_config(_config_rom.xml());
+			});
 		}
 
 	protected:
@@ -92,7 +94,7 @@ class Terminal::Root_component : public Genode::Root_component<Session_component
 
 		void _destroy_session(Session_component *s)
 		{
-			_server.detach_terminal(*s);
+			Libc::with_libc([&] () { _server.detach_terminal(*s); });
 			Genode::destroy(md_alloc(), s);
 		}
 

repos/gems/src/server/ssh_terminal/server.cc

@@ -235,11 +235,13 @@ void Ssh::Server::_parse_config(Genode::Xml_node const &config)
 	_log_level  = config.attribute_value("debug",      0u);
 	_log_logins = config.attribute_value("log_logins", true);
 
-	Genode::Mutex::Guard guard(_logins.mutex());
+	{
-	auto print = [&] (Login const &login) {
+		Util::Pthread_mutex::Guard guard(_logins.mutex());
-		Genode::log("Login configured: ", login);
+		auto print = [&] (Login const &login) {
-	};
+			Genode::log("Login configured: ", login);
-	_logins.for_each(print);
+		};
+		_logins.for_each(print);
+	}
 
 	if (_config_once) { return; }
 
@@ -331,7 +333,7 @@ void Ssh::Server::_log_login(User const &user, Session const &s, bool pubkey)
 
 void Ssh::Server::attach_terminal(Ssh::Terminal &conn)
 {
-	Genode::Mutex::Guard guard(_terminals.mutex());
+	Util::Pthread_mutex::Guard guard(_terminals.mutex());
 
 	try {
 		new (&_heap) Terminal_session(_terminals,
@@ -359,7 +361,7 @@ void Ssh::Server::attach_terminal(Ssh::Terminal &conn)
 
 void Ssh::Server::detach_terminal(Ssh::Terminal &conn)
 {
-	Genode::Mutex::Guard guard(_terminals.mutex());
+	Util::Pthread_mutex::Guard guard(_terminals.mutex());
 
 	Terminal_session *p = nullptr;
 	auto lookup = [&] (Terminal_session &t) {
@@ -380,10 +382,8 @@ void Ssh::Server::detach_terminal(Ssh::Terminal &conn)
 		sess.terminal_detached = true;
 
 		/* flush before destroying the terminal */
-		Libc::with_libc([&] {
+		try { sess.terminal->send(sess.channel); }
-			try { sess.terminal->send(sess.channel); }
+		catch (...) { }
-			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;
-		char c = 1;
+	::write(_server_fds[1], &c, sizeof(c));
-		::write(_server_fds[1], &c, sizeof(c));
-	});
 }
 
 
 static int write_avail_cb(socket_t fd, int revents, void *userdata)
 {
-	int n = 0;
+	char c;
-	Libc::with_libc([&] {
+	return ::read(fd, &c, sizeof(char));
-		char c;
-		n = ::read(fd, &c, sizeof(char));
-	});
-	return n;
 }

repos/gems/src/server/ssh_terminal/server.h

@@ -61,14 +61,6 @@ struct Ssh::Session : Genode::Registry<Session>::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<Session> &reg,
 	        ssh_session s,
 	        ssh_channel_callbacks ccb,
@@ -111,8 +103,8 @@ struct Ssh::Terminal_session : Genode::Registry<Terminal_session>::Element
 
 struct Ssh::Terminal_registry : Genode::Registry<Terminal_session>
 {
-	Genode::Mutex _mutex { };
+	Util::Pthread_mutex _mutex { };
-	Genode::Mutex &mutex() { return _mutex; }
+	Util::Pthread_mutex &mutex() { return _mutex; }
 };
 
 

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 };
+	Ssh::Terminal &conn              { *p->terminal };
-	Mutex::Guard   guard     { conn.read_buf.mutex() };
+	Util::Pthread_mutex::Guard guard { conn.read_buf.mutex() };
-	char const    *src       { reinterpret_cast<char const*>(data) };
+	char const    *src               { reinterpret_cast<char const*>(data) };
-	size_t         num_bytes { 0 };
+	size_t         num_bytes         { 0 };
 
 	while ((conn.read_buf.write_avail() > 0) && (num_bytes < len)) {
 

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());
+			bool empty = true;
-			return !read_buf.read_avail();
+
+			Libc::with_libc([&] {
+				Util::Pthread_mutex::Guard guard(read_buf.mutex());
+				empty = !read_buf.read_avail();
+			});
+
+			return empty;
 		}
 };
 

repos/gems/src/server/ssh_terminal/util.h

@@ -21,6 +21,7 @@
 #include <libc/component.h>
 
 /* libc includes */
+#include <pthread.h>
 #include <unistd.h>
 #include <time.h>
 
@@ -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 <size_t C>
 struct Util::Buffer
 {
-	Genode::Mutex _mutex   { };
+	Util::Pthread_mutex _mutex   { };
-	char          _data[C] { };
+	char                _data[C] { };
-	size_t        _head    { 0 };
+	size_t              _head    { 0 };
-	size_t        _tail    { 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_ */