Differential D13659 Diff 37487 vnc/vncsshtunnelthread.cpp

Changeset View

Standalone View

View Options

vnc/vncsshtunnelthread.cpp

  • This file was added.
1​/****************************************************************************
2​**
3​** Copyright (C) 2018 Klarälvdalens Datakonsult AB, a KDAB Group
4​** company, info@kdab.com. Work sponsored by the
5​** LiMux project of the city of Munich
6​**
7​** This file is part of KRDC.
8​**
9​** This program is free software; you can redistribute it and/or modify
10​** it under the terms of the GNU General Public License as published by
11​** the Free Software Foundation; either version 2 of the License, or
12​** (at your option) any later version.
13​**
14​** This program is distributed in the hope that it will be useful,
15​** but WITHOUT ANY WARRANTY; without even the implied warranty of
16​** MEresHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17​** GNU General Public License for more details.
18​**
19​** You should have received a copy of the GNU General Public License
20​** along with this program; see the file COPYING. If not, write to
21​** the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
22​** Boston, MA 02110-1301, USA.
23​**
24​****************************************************************************/
25​
26​#include "vncsshtunnelthread.h"
27​#include "krdc_debug.h"
28​
29​#include <KLocalizedString>
30​
31​#include <fcntl.h>
32​#include <arpa/inet.h>
33​#include <netinet/in.h>
34​#include <sys/socket.h>
35​
36​#include <QDebug>
37​
38​VncSshTunnelThread::VncSshTunnelThread(const QByteArray &host, int vncPort, int tunnelPort, int sshPort, const QByteArray &sshUserName, bool loopback) :
39​ m_host(host),
40​ m_vncPort(vncPort),
41​ m_tunnelPort(tunnelPort),
42​ m_sshPort(sshPort),
43​ m_sshUserName(sshUserName),
44​ m_loopback(loopback),
45​ m_stop_thread(false)
46​{
47​}
48​
49​VncSshTunnelThread::~VncSshTunnelThread()
50​{
51​ m_stop_thread = true;
52​ wait();
53​}
54​
55​
56​QString VncSshTunnelThread::password() const
57​{
dfaureUnsubmitted
Done

I would add a comment here, for the next reader.

// This is called by the main thread, but from a slot connected to our signal via BlockingQueuedConnection, so this is safe even without a mutex, the semaphore in BlockingQueuedConnection takes care of the synchronization.

dfaure: I would add a comment here, for the next reader. // This is called by the main thread, but…
58​ return m_password;
59​}
60​
61​// This is called by the main thread, but from a slot connected to our signal via BlockingQueuedConnection
62​// so this is safe even without a mutex, the semaphore in BlockingQueuedConnection takes care of the synchronization.
63​void VncSshTunnelThread::setPassword(const QString &password, PasswordOrigin origin)
64​{
65​ m_password = password;
66​ m_passwordOrigin = origin;
67​}
68​
69​void VncSshTunnelThread::run()
70​{
71​ struct CleanupHelper
72​ {
73​ int server_sock = -1;
74​ int client_sock = -1;
75​ ssh_session session = nullptr;
76​ ssh_channel forwarding_channel = nullptr;
77​
78​ ~CleanupHelper()
79​ {
80​ // the ssh functions just return if the param is null
81​ ssh_channel_free(forwarding_channel);
82​ if (client_sock != -1) {
83​ close(client_sock);
84​ }
85​ if (server_sock != -1) {
86​ close(server_sock);
87​ }
88​ ssh_disconnect(session);
89​ ssh_free(session);
90​ }
91​ };
92​
93​ CleanupHelper cleanup;
94​
95​ ssh_session session = ssh_new();
96​ if (session == nullptr)
97​ return;
98​
99​ cleanup.session = session;
100​
101​ ssh_options_set(session, SSH_OPTIONS_HOST, m_host.constData());
102​ ssh_options_set(session, SSH_OPTIONS_USER, m_sshUserName.constData());
103​ ssh_options_set(session, SSH_OPTIONS_PORT, &m_sshPort);
104​
105​ int res = ssh_connect(session);
106​ if (res != SSH_OK)
107​ {
108​ emit errorMessage(i18n("Error connecting to %1: %2", QString::fromUtf8(m_host), QString::fromLocal8Bit(ssh_get_error(session))));
109​ return;
110​ }
111​
112​ // First try authenticating via ssh agent
113​ res = ssh_userauth_agent(session, NULL);
114​
115​ if (res != SSH_AUTH_SUCCESS) {
116​ // If ssh agent didn't work, try with password
117​ emit passwordRequest(NoFlags); // This calls blockingly to the main thread which will call setPassword
118​ res = ssh_userauth_password(session, NULL, m_password.toUtf8().constData());
119​
120​ // If password didn't work but came from the wallet, ask the user for the password
121​ if (res != SSH_AUTH_SUCCESS && m_passwordOrigin == PasswordFromWallet) {
uwolferUnsubmitted
Not Done

(whitespace after 'if' is not consistent with other parts of this file)

uwolfer: (whitespace after 'if' is not consistent with other parts of this file)
aacidAuthorUnsubmitted
Not Done

will fix on commit/if i upload a new revision, just don't feel there's a need to upload a new change because just of this.

aacid: will fix on commit/if i upload a new revision, just don't feel there's a need to upload a new…
122​ emit passwordRequest(IgnoreWallet); // This calls blockingly to the main thread which will call setPassword
123​ res = ssh_userauth_password(session, NULL, m_password.toUtf8().constData());
124​ }
125​ }
126​
127​ if (res != SSH_AUTH_SUCCESS) {
128​ emit errorMessage(i18n("Error authenticating with password: %1", QString::fromLocal8Bit(ssh_get_error(session))));
129​ return;
130​ }
dfaureUnsubmitted
Done

(missing space after if, here as well)

dfaure: (missing space after if, here as well)
131​
132​ const int server_sock = socket(AF_INET, SOCK_STREAM, 0);
133​ if (server_sock == -1) {
dfaureUnsubmitted
Done

You could use a local RAII class to avoid duplicating close+ssh_disconnect+ssh_free in every return code path (initialize m_server_sock to -1 in the constructor so you can test for it before calling close).

As a benefit this huge method would be a little bit less huge ;)

dfaure: You could use a local RAII class to avoid duplicating close+ssh_disconnect+ssh_free in every…
134​ emit errorMessage(i18n("Error creating tunnel socket"));
135​ return;
136​ }
137​
138​ cleanup.server_sock = server_sock;
139​
140​ // so that we can bind more than once in case more than one tunnel is used
141​ int sockopt = 1;
142​ setsockopt(server_sock , SOL_SOCKET, SO_REUSEADDR, &sockopt, sizeof(sockopt));
143​
144​ {
145​ // bind the server socket
146​ struct sockaddr_in sin;
147​ sin.sin_family = AF_INET;
148​ sin.sin_port = htons(m_tunnelPort);
dfaureUnsubmitted
Done

"any error here will propagate" confused me, I was wondering how those two threads would communicate then.
After your clarification over chat, I think this comment should say something like "any socket error here will be detected by the vnc thread".

dfaure: "any error here will propagate" confused me, I was wondering how those two threads would…
149​ sin.sin_addr.s_addr = inet_addr("127.0.0.1");
150​
151​ if (bind(server_sock, (struct sockaddr*)&sin, sizeof sin) == -1) {
152​ emit errorMessage(i18n("Error creating tunnel socket"));
153​ return;
154​ }
155​ }
156​
157​ if (listen(server_sock, 1) == -1) {
158​ emit errorMessage(i18n("Error creating tunnel socket"));
159​ return;
160​ }
161​
162​ if (m_stop_thread) {
163​ return;
164​ }
165​
166​ emit listenReady();
167​ // After here we don't need to emit errorMessage anymore on error, qCDebug is enough
168​ // this is because the actual vnc thread will start because of this call and thus
169​ // any socket error here will be detected by the vnc thread and the usual error mechanisms
170​ // there will warn the user interface
171​
172​ int client_sock;
173​ {
174​ struct sockaddr_in client_sin;
175​ socklen_t client_sin_len = sizeof client_sin;
176​ client_sock = accept(server_sock, (struct sockaddr *)&client_sin, &client_sin_len);
177​ if (client_sock == -1)
178​ {
179​ qCDebug(KRDC) << "Error on tunnel socket accept";
180​ return;
181​ }
182​
183​ cleanup.client_sock = client_sock;
184​
185​ int sock_flags = fcntl(client_sock, F_GETFL, 0);
186​ fcntl(client_sock, F_SETFL, sock_flags | O_NONBLOCK);
187​ }
188​
189​ ssh_channel forwarding_channel = ssh_channel_new(session);
dfaureUnsubmitted
Not Done

missing space after while
one space too many after eof...

Should this also say while (!error && ...)? I'm not sure, but it looks like the current code might go into an infinite loop if FD_ISSET always return false for some reason.
OTOH I'm not sure if you want to abort on first error...

dfaure: missing space after while one space too many after eof... Should this also say while (!error…
aacidAuthorUnsubmitted
Not Done

spacing fixed, about adding the error to the while, i really don't know, i *think* if something fails here, the actual vnc part of the code will fail too since it won't get reads/writes and everything will end up failing nicely at some point.

But it's one of this things that are really hard to test since i don't really know how to make it fail properly, so i'd like to leave it as is.

aacid: spacing fixed, about adding the error to the while, i really don't know, i *think* if something…
190​ {
191​ const char *forward_remote_host = m_loopback ? "127.0.0.1" : m_host.constData();
192​ res = ssh_channel_open_forward(forwarding_channel, forward_remote_host, m_vncPort, "127.0.0.1", 0);
193​ if (res != SSH_OK || !ssh_channel_is_open(forwarding_channel))
194​ {
195​ qCDebug(KRDC) << "SSH channel open error" << ssh_get_error(session);
196​ return;
197​ }
198​ cleanup.forwarding_channel = forwarding_channel;
199​ }
200​
201​ char client_read_buffer[40960];
202​ char *channel_read_buffer = nullptr;
203​ char *channel_read_buffer_ptr = nullptr;
204​ int channel_read_buffer_to_write;
205​ while (!m_stop_thread && !ssh_channel_is_eof(forwarding_channel)) {
206​ struct timeval timeout;
207​ timeout.tv_sec = 0;
208​ timeout.tv_usec = 200000;
209​
210​ fd_set set;
211​ FD_ZERO(&set);
212​ FD_SET(client_sock, &set);
213​ ssh_channel channels[2] = { forwarding_channel, NULL };
214​ ssh_channel channels_out[2] = { NULL, NULL };
215​
216​ res = ssh_select(channels, channels_out, client_sock + 1, &set, &timeout);
217​ if (res == SSH_EINTR) continue;
218​ if (res == -1) break;
219​
220​ bool error = false;
221​ if (FD_ISSET(client_sock, &set)) {
222​ int bytes_read;
223​ while (!error && (bytes_read = read(client_sock, client_read_buffer, sizeof client_read_buffer)) > 0) {
224​ int bytes_written = 0;
225​ int bytes_to_write = bytes_read;
226​ for (char *ptr = client_read_buffer;
227​ bytes_to_write > 0;
228​ bytes_to_write -= bytes_written, ptr += bytes_written)
229​ {
230​ bytes_written = ssh_channel_write(forwarding_channel, ptr, bytes_to_write);
231​ if (bytes_written <= 0) {
232​ error = true;
233​ qCDebug(KRDC) << "error on ssh_channel_write";
dfaureUnsubmitted
Done

Instead of the if+continue above, this line could say if (!error && !channel_read_buffer), to match the next if() which also does if (!error && ...). It just seems asymetric to use continue in one case and if in the other.

dfaure: Instead of the if+continue above, this line could say if (!error && !channel_read_buffer), to…
234​ break;
235​ }
236​ }
237​ }
238​ if (bytes_read == 0) {
239​ qCDebug(KRDC) << "error on tunnel read";
240​ error = true;
241​ }
242​ }
243​
244​ // If on the previous iteration we successfully wrote all we read, we need to read again
245​ if (!error && !channel_read_buffer) {
246​ const int bytes_available = ssh_channel_poll(forwarding_channel, 0);
247​ if (bytes_available == SSH_ERROR || bytes_available == SSH_EOF) {
248​ qCDebug(KRDC) << "error on ssh_channel_poll";
249​ error = true;
250​ } else if (bytes_available > 0) {
251​ channel_read_buffer = new char[bytes_available];
252​ channel_read_buffer_ptr = channel_read_buffer;
253​ const int bytes_read = ssh_channel_read_nonblocking(forwarding_channel, channel_read_buffer, bytes_available, 0);
254​ if (bytes_read <= 0) {
255​ qCDebug(KRDC) << "error on ssh_channel_read_nonblocking";
256​ error = true;
257​ } else {
258​ channel_read_buffer_to_write = bytes_read;
259​ }
260​ }
261​ }
262​
263​ if (!error && channel_read_buffer) {
264​ for (int bytes_written = 0;
265​ channel_read_buffer_to_write > 0;
266​ channel_read_buffer_to_write -= bytes_written, channel_read_buffer_ptr += bytes_written)
267​ {
268​ bytes_written = write(client_sock, channel_read_buffer_ptr, channel_read_buffer_to_write);
269​ if (bytes_written == -1 && errno == EAGAIN) {
dfaureUnsubmitted
Not Done

On error, this delete[] might never happen. Maybe do it at end of block too?

dfaure: On error, this delete[] might never happen. Maybe do it at end of block too?
aacidAuthorUnsubmitted
Not Done

Added outside the while

aacid: Added outside the while
270​ // socket is full, just carry on and we will write on the next iteration
271​ // that is why the previous code does 'if (!channel_read_buffer)'
272​ break;
273​ }
274​ if (bytes_written <= 0) {
275​ qCDebug(KRDC) << "error on tunnel write";
276​ error = true;
277​ break;
278​ }
279​ }
280​ if (channel_read_buffer_to_write <= 0) {
281​ delete[] channel_read_buffer;
282​ channel_read_buffer = nullptr;
283​ }
284​ }
285​ }
dfaureUnsubmitted
Not Done

This is for sure a race on m_server_sock. I guess this is here in order to wake up ssh_select in the thread?

In that case, m_server_sock needs to be mutex-protected, everywhere it's used.
But the run() method could use a local variable with a copy of that socket value, so that it only needs mutex-protection to write into the member variable, once, when the value is first known.

But actually, ssh_select has a timeout of 200ms so m_stop_thread is checked every 200ms, no?
(not sure why it needs to wake up so often, though, especially if we can stop the thread by closing the socket).

It seems to me that either the timeout stays short and stop() can just wait 200ms, or the timeout becomes much longer and we do need to shutdown here. Am I missing something?

dfaure: This is for sure a race on m_server_sock. I guess this is here in order to wake up ssh_select…
aacidAuthorUnsubmitted
Not Done

Actually i don't think i need the shutdown call anymore, i think it's back from when i had a bit of a different code structure and the thread was getting blocked in accept() if i could not connect and i needed to nudge it out to be able to stop the thread, but i reworked that a while ago and i don't really need it anymore (or at least could not reproduce it getting stuck and previously was very easy)

So i've just removed the shutdown and then the race is gone :)

aacid: Actually i don't think i need the shutdown call anymore, i think it's back from when i had a…
286​
287​ delete[] channel_read_buffer;
288​ channel_read_buffer = nullptr;
289​}