RCC_FOREACH may be dangerous
The following patches replace FOREACH loops with a SAFE version.
Using unsafe loops may cause spice-server to abort (assert fails).
Specifically a read/write fail in those loops, may cause the client
to disconnect, removing the node currently iterated, which cause spice
to abort in ring_next():
-- assertion `pos->next != NULL && pos->prev != NULL' failed
Currently, both red_channel_pipes_add_type() and
red_channel_pipes_add_empty_msg() use plaing RING_FOREACH() which is not
safe versus removals from the ring within the loop body.
Although it's rare, such a removal can occur in both cases. In the case
of red_channel_pipes_add_type() we have:
red_channel_pipes_add_type()
-> red_channel_client_pipe_add_type()
-> red_channel_client_push()
And in the case of red_channel_client_pipes_add_empty_msg() we have:
red_channel_client_pipes_add_empty_msg()
-> red_channel_client_pipe_add_empty_msg()
-> red_channel_client_push()
But red_channel_client_push() can cause a removal from the clients ring if
a network error occurs:
red_channel_client_push()
-> red_channel_client_send()
-> red_peer_handle_outgoing()
-> handler->cb->on_error callback
= red_channel_client_default_peer_on_error()
-> red_channel_client_disconnect()
-> red_channel_remove_client()
-> ring_remove()
When this error path does occur, the assertion in RING_FOREACH()'s
ring_next() trips, and the process containing the spice server is aborted.
i.e. your whole VM dies, as a result of an unfortunately timed network
error on the spice channel.
Please apply.
Signed-off-by: David Gibson <dgibson@redhat.com>
The image descriptor flags shouldn't be copied as is from the flags that
were set by the driver. Specifically, the CACHE_ME flag shouldn't be copied,
since it is possible that (a) the image won't be cached (b) the image
is already cached, but in its lossy version, and we may want to set the bit for
CACHE_REPLACE_ME, in order to cache it in its lossless version.
In case (b), the client first looks for the CACHE_ME flag, and only if
it is not set it looks for CACHE_REPLACE_ME (see canvas_base.c). Since both flags where set,
the client ignored REPLACE_ME, and didn't turned off the lossy flag of the
cach item. Then, when a request from this lossles item reached the
client (FROM_CACHE_LOSSLESS), the client display channel waited
endlessly for the lossless version of the image.
The assert:
spice_assert(pthread_equal(pthread_self(), client->thread_id))
and the assert:
spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id))
were coded in order to protect data that is accessed from the main
context (red_client and most of the channels), from
access by threads of other channels (namely, the display and cursor
channels), and vice versa.
However, some of the calls to the sound channel interface,
and also the char_device interface, can be done from the vcpu thread.
It doesn't endanger these channels internal data, since qemu use global
mutex for the vcpu and io threads.
Thus, pthread_self() can be != channel->thread_id, if one of them is
the vcpu thread and the other is the io-thread, and we shouldn't assert.
Future plans: A more complete and complicated solution would be to manage our own thread for
spice-channels, and push input from qemu to this thread, instead of
counting on the global mutex of qemu
rhbz#823472
If client_migrate_info was called once with cert-host-subject and
then again without cert-host-subject, on a third call to
client_migrate info, the cert-host-subject from the first call would
have been freed for the second time.
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.
When setting an initial video stream bit rate, if the bit rate
wasn't calculated by main_channel_client, and we don't have
estimation from previos streams, use some default values.
The patch also removes updating dcc->streams_max_bit_rate when
the bit_rate held by the main_channel is larger than it. It is not necessary
since we compare those 2 values each time we set the initial bit rate
for a stream.
spice_channel_client_error prints warning and shutdowns the
channel_client that hit the error.
This macro is useful for errors that are specific for one session
and that are unrecoverable only with respect to this session.
Prefer disconnecting a client over aborting when possible.
rhbz#956345
After a spice session has been migrated, we don't retest the network
(user experience considerations). Instead, we obtain the is_low_bandwidth flag
from the src-server, via the migration data.
Before this patch, if we migrated from server s1 to s2 and then to s3,
and if the connection to s1 was a low bandwidth one, we erroneously
passed is_low_bandwidth=FALSE from s2 to s3.
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Replace the mixed calls to display_channel_client_is_low_bandwidth
and to main_channel_client_is_low_bandwidth, with one flag in
CommonChannelClient that is set upon channel creation.
red_create_stream is called even without any client but there is no
encoding since the mjpeg encoder is now associated with StreamAgent
which is only created when we have a client.
The connection to the target server is established before migration
starts. However, the client reads and replies to messages from the server only after
migration completes. Thus, we better not send ping msgs from the target
before migration completes (because the observed roundtrip duration will
be bigger than the real one).
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 a client disconnects, red_channel_client_pipe_clear is called.
Releasing pipe items of type == MIGRATE||EMPTY_MSG||PING
wasn't handled, and was passed to channel_cbs.release_item.
There, an error occured since the pipe items were not recognized.
With a SPICE_DISPLAY_CAP_MONITORS_CONFIG capable client, the client needs to
know what part of the primary to use for each monitor. If the guest driver
does not support this, the server sends messages to the client for a
single monitor spanning the entire primary.
As soon as the guest calls spice_qxl_monitors_config_async once, we set
the red_worker driver_has_monitors_config flag and stop doing this.
This is a problem when the driver gets unloaded, for example after a reboot
or when switching to a text vc with usermode mode-setting under Linux.
To reproduce this start a multi-mon capable Linux guest which uses
usermode mode-setting and then once X has started switch to a text vc. Note
how the client window does not only not resize, if you try to resize it
manually you always keep blackborders since the aspect is wrong.
This patch is the spice-server side of fixing this, it adds a new
spice_qxl_driver_unload method which clears the driver_has_monitors_config
flag.
The other patch needed to fix this is in qemu, and will calls this new method
from qxl_enter_vga_mode.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
mjpeg_encoder modify the initial bit we supply it, according to the
client feedback. If it reaches a bit rate which is higher than the
initial one, we use the higher bit rate as the new bit rate estimation.
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.
A frame can be dropped if a new frame was added during the same
call to red_process_command (we didn't attempt to send the older
frame). Such drops are ignored.
This patch only employs setting the stream parameters based on
the initial given bit-rate, the latency, and the encoding size.
Later patches will also employ mjpeg_encoder response to client reports,
and its control over frame drops.
The patch also removes old stream bit rate calculations that weren't
used.
