SASL authentication mostly use members from RedsStream to do its work, so
it makes sense to have its code in reds_stream.c. This should allow to make
RedsStream::sasl private in the future.
The AsyncRead structure in reds.h wraps an async read + callback to
be done on a stream. Moving it to reds_stream.h is needed in order
to move SASL authentication there.
Now that stream creation and SSL enabling are done by helpers
in reds_stream.c, we can move the initialization of the vfunc
read/write pointers there too.
Initializing a new stream means initializing quite a few fields.
This commit factors this initialization in a dedicated reds_stream_new
helper. This also helps moving more code from reds.c to reds_stream.c
When creating a TLS socket, both spice-server and spice-gtk currently
call SSL_CTX_new(TLSv1_method()). The TLSv1_method() function set the
protocol version to TLS 1.0 exclusively. The correct way to support
multiple protocol versions is to call SSLv23_method() in spite of its
scary name. This method will enable all SSL/TLS protocol versions. The
protocol suite may be further narrowed down by setting respective
SSL_OP_NO_<version_code> options of SSL context. This possibility is
used in this patch in order to block use of SSLv3 that is enabled by
default in openssl for client sockets as of now but spice has never used
it.
reds_handle_ticket uses a fixed size 'password' buffer for the decrypted
password whose size is SPICE_MAX_PASSWORD_LENGTH. However,
RSA_private_decrypt which we call for the decryption expects the
destination buffer to be at least RSA_size(link->tiTicketing.rsa)
bytes long. On my spice-server build, SPICE_MAX_PASSWORD_LENGTH
is 60 while RSA_size() is 128, so we end up overflowing 'password'
when using long passwords (this was reproduced using the string:
'fullscreen=1proxy=#enter proxy here; e.g spice_proxy = http://[proxy]:[port]'
as a password).
When the overflow occurs, QEMU dies with:
*** stack smashing detected ***: qemu-system-x86_64 terminated
This commit ensures we use a corectly sized 'password' buffer,
and that it's correctly nul-terminated so that we can use strcmp
instead of strncmp. To keep using strncmp, we'd need to figure out
which one of 'password' and 'taTicket.password' is the smaller buffer,
and use that size.
This fixes rhbz#999839
It's depending on an unmaintained package (slirp), and I don't
think anyone uses that code. It's not tested upstream nor in fedora,
so let's remove it.
When we want to disconnect the main channel from a callback, it is
safer to use red_channel_client_shutdown, instead of directly
destroying the client. It is also more consistent with how other
channels treat errors.
red_channel_client_shutdown will trigger socket error in the main channel.
Then, main_channel_client_on_disconnect will be called,
and eventually, main_dispatcher_client_disconnect.
I didn't replace calls to reds_disconnect/reds_client_disconnect in
places where those calls were safe && that might need immediate client
disconnection.
Fixes rhbz#918169
Some channels make direct calls to reds/main_channel routines. If
these routines try to read/write to the socket, and they get socket
error, main_channel_client_on_disconnect is called, and triggers
red_client_destroy. In order to prevent accessing expired references
to RedClient, RedChannelClient, or other objects (inside the original call, after
red_client_destroy has been called) I made the call to
red_client_destroy asynchronous with respect to main_channel_client_on_disconnect.
I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher.
main_channel_client_on_disconnect pushes this msg to the dispatcher,
instead of calling directly to reds_client_disconnect.
The patch uses RedClient ref-count in order to handle a case where
reds_client_disconnect is called directly (e.g., when a new client connects while
another one is connected), while there is already CLIENT_DISCONNECT msg
pending in the main_dispatcher.
Examples:
(1) snd_worker.c
snd_disconnect_channel()
channel->cleanup() //snd_playback_cleanup
reds_enable_mm_timer()
.
.
main_channel_push_multi_media_time()...socket_error
.
.
red_client_destory()
.
.
snd_disconnect_channel()
channel->cleanup()
celt051_encoder_destroy()
celt051_encoder_destory() // double release
Note that this bug could have been solved by changing the order of
calls: e.g., channel->stream = NULL before calling cleanup, and
some other changes + reference counting. However, I found other
places in the code with similar problems, and I looked for a general
solution, at least till we redesign red_channel to handle reference
counting more consistently.
(2) inputs_channel.c
inputs_connect()
main_channel_client_push_notify()...socket_error
.
.
red_client_destory()
.
.
red_channel_client_create() // refers to client which is already destroyed
(3) reds.c
reds_handle_main_link()
main_channel_push_init() ...socket error
.
.
red_client_destory()
.
.
main_channel_client_start_net_test(mcc) // refers to mcc which is already destroyed
This can explain the assert in rhbz#964136, comment #1 (but not the hang that occurred before).
It's not always obvious what address spice-server will bind to,
in particular when the 'addr' parameter is omitted on QEMU
commandline. The decision of what address to bind to is made
in reds_init_socket with a call to getaddrinfo. Surprisingly,
that function had a call to getnameinfo() already, but it does
not seem to be using the result of that call in any way.
This commit moves this call after the socket is successfully bound
and add a log message to indicate which address it's bound to.
This bug results in the client dropping all the video frames after
migration in case that (1) the hosts involved in migration have different
mm-time; and that (2) there is no audio playback.
This is relvant only for the client that was connected during the
migration.
rhbz#958276
When there is no audio playback, we set the mm_time in the client to be older
than the one in the server by at least the requested latency (the delta is
actually bigger, due to the network latency).
When there is an audio playback, we adjust the mm_time in the client by
adjusting the playback buffer using SPICE_MSG_PLAYBACK_LATENCY.
This is clearly something which should be handled in the inputs_channel code,
rather then having a special case for it in the generic channel handling
code in reds.c. Moving it here also fixes the TODO we had on only sending
this message to new clients.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Currently main_channel_push_notify only gets passed a static string, but
chances are in the future it may get passed dynamically allocated strings,
prepare it for this.
While at it also make clear that its argument is a string, and simplify
things a bit by making use of this knowledge (pushing the strlen call down).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Client -> agent messages can spawn multiple VDIChunks. When this happens
the agent re-assembles the chunks into a complete VDAgentMessage before
processing it. The server only guarentees coherency at the chunk level,
so it is not possible for a partial chunk to get delivered to the agent.
But it is possible for some chunks of a VDAgentMessage to be delivered to
the agent followed by a client to disconnect without the rest of the
VDAgentMessage being delivered!
This will leave the agent in a wrong state, and the first messages send to it
by the next client to connect will get seen as the rest of the VDAgentMessage
from the previous client.
This patch sends the agent a new VD_AGENT_CLIENT_DISCONNECTED message from the
VDP_SERVER_PORT, on which the agent can then reset its VDP_CLIENT_PORT state.
Note that no capability check is done for this, since the capabilities are
something negotiated between client and agent. The server will simply always
send this message on client disconnect, relying on older agents discarding the
message since it has an unknown type (which both the windows and linux agents
already do).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
reds.c is using strncpy with a length one byte less than the
destination buffer size, and is relying on the fact that the
destination buffers are static global variables.
Now that we depend on glib, we can use g_strlcpy instead, which
avoids relying on such a subtle trick to get a nul-terminated
string.
We currently output a warning when getaddrinfo fails, but then
we go on trying to use the information it couldn't read. Make
sure we bail out of reds_init_socket if getaddrinfo fails.
spice_server_set_ticket and spice_server_set_addr get (library)
user-provided strings as arguments, and copy them to fixed-size
buffers using strncpy. However, if these strings are too long,
the copied string will not be 0-terminated, which will cause issues
later. This commit copies one byte less than the size of the
destination buffer. In both cases, this buffer is a static global
variable, so its memory will be set to 0.
A Spice port channel carry arbitrary data between the Spice client and
the Spice server. It may be used to provide additional services on top
of a Spice connection. For example, a channel can be associated with
the qemu monitor for the client to interact with it, just like any
qemu chardev. Or it may be used with various protocols, such as the
Spice Controller.
A port kind is identified simply by its fqdn, such as org.qemu.monitor,
org.spice.spicy.test or org.ovirt.controller...
The channel is based on Spicevmc which simply tunnels data between
client and server, with a few additional messages.
See the description of the channel protocol in spice-common history.
The server can receive from the client agent data even when the agent
is disconnected. This can happen if the client sends the agent data
before it receives the AGENT_DISCONNECTED msg. We should receive and handle such msgs, instead
of disconnecting the client.
This bug can also lead to a server crash if the agent gets reconnected
fast enough, and it receives an agent data msg from the client before MSGC_AGENT_START.
upstream bz#55726
rhbz#881980
New API: spice_server_set_ws_ports
This adds an optional dependency on libwebsockets. You need to get my
patched 0.0.3 version here:
git://people.freedesktop.org/~alon/libwebsockets
There is no qemu patches yet, to test change in reds.c the default value
of spice_ws_port to 5959 (for the default of spice-html5).
For testing there is an online client at
http://spice-space.org/spice-html5/spice.html
Known issues:
1. The tester (server/tests/test_display_no_ssl) gets into dropping all
data after a few seconds, I think it's an issue with the implemented
watches, but haven't figured it out.
2. libwebsocket's read interface is inverted to what our code expects,
i.e. there is no libwebsocket_read, so there is an additional copy
involved (see RedsWebSocket). This can be fixed.
3. Listening on a separate port. Since the headers are different, we
could listen on the same port (first three bytes RED/GET). I don't know
if we want to?
Todos:
1. SSL not implemented yet. Needs some thought as to how.
2. Serve spice-html5 when accessed as a http server. Nice to have.
This used to abort with spice_error. The caller currently does
not check spice_server_char_device_add_interface return value, but
it's still cleaner to report an error in this case.
Errors occurring in reds_init_ssl used to be fatal through the use
of spice_error, but this was downgraded to non-fatal spice_warning
calls recently. This means we no longer error out when invalid SSL
(certificates, ...) parameters are passed by the user.
This commit changes reds_init_ssl return value from void to int so
that errors can be reported to the caller.
We used to be aborting in such situations, but this was changed
during the big spice_error/printerr cleanup. We are currently
outputting a warning but not reporting the error with the caller
when reds_init_net fails to register listening watches with the
mainloop. As it's unlikely that things will work as expected in
such cases, better to error out of the function instead of pretending
everything is all right.
BN_new returns NULL on allocation failures. Given that we abort
on malloc allocation failures, we should also abort here. The
current code will segfault when BN_new fails as it immediatly tries
to use the NULL pointer.
If the guest supports client monitors config we pass it the
VDAgentMonitorsConfig message via the
QXLInterface::client_monitors_config api instead of via the vdagent.
If reading/writing from the device have occured before migration data
has arrived, the migration data might no longer be relvant, and we
disconnect the client.
