For some reasons (documented in cursor_init) the function
uses 128 extra bytes of data causing a reading buffer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Timers in spice server are supposed to be used in a single thread
context. To avoid problems, protect the usage from multiple
thread with a mutex.
This sometimes caused a crash.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The wakeup timer is used by the worker thread and by the
main thread.
Destroying the object before destroying the worker thread
can lead to use after free.
Destroying the worker thread first makes sure we don't race.
This is detected easily when compiling the test with address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As the indexes are used to compute the index inside an array
using modulo operation when a signed value overflows, the
modulo becames negative, causing a buffer underflow.
Unlikely to happens (take lot of time) but is safer that way.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Cursor resources (basically the shape of it) was retained till
it was used however it was copied so there were no reason to not release
this resource.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
You could easily trigger this issue using multiple monitors and
a modified spice-gtk client with this patch:
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
{
g_return_val_if_fail(c != NULL, FALSE);
+ if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
spice_channel_new(c->session, c->type, c->id);
g_object_unref(c->session);
This as g_initable_new in this case returns NULL (dcc.c).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Although dispatcher_send_message() does not allow you to send a message
type that is invalid for a dispatcher, it still makes sense to verify
that the type is valid in the receiver. This should only be possible in
the case of some severe problem such as memory corruption, so if it is
invalid, we simply abort.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Is possible to have a leak processing update commands if
the update command is synchronous and the rectangle list
is empty. Note that Qemu always pass an empty list.
If the list is empty display_channel_update fill the list.
This is used to send back the list in case of asynchronous
requests. But in handle_dev_update_async (the callback that
handle the asynchronous case) the list is correctly freed.
This was discovered by accident looking at the code.
Reproduced with a Windows recording file using GCC address
sanitizer and this patch to spice-server-replay:
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -1280,7 +1280,13 @@ static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
replay->created_primary = FALSE;
worker->destroy_surfaces(worker);
break;
- case RED_WORKER_MESSAGE_UPDATE:
+ case RED_WORKER_MESSAGE_UPDATE: {
+ static uint8_t count = 0;
+ QXLRect dummy;
+ QXLRect update = { 0, 0, 100, 100 };
+ count ^= 1;
+ worker->update_area(worker, 0, &update, count ? &dummy : NULL, count ? 1 : 0, 0);
+ } break;
// XXX do anything? we record the correct bitmaps already.
case RED_WORKER_MESSAGE_DISPLAY_CONNECT:
// we want to ignore this one - it is sent on client connection, we
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedPipeItem already implements reference counting so
this avoid duplicating code to handle a object with reference
counting that points to another object with reference counting
that holds a RedCursorCmd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required
only if you are handling data in a loop instead of using the
default loop.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Implements red_channel_pipes_add_type and
red_channel_pipes_add_empty_msg using red_channel_pipes_add.
This avoid duplicating items for each client.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Considering that now RedPipeItem have reference counting
and that lot of items are just used to store constant
data to send, using reference counting instead of creating
different items for each client is easier to do.
So this new red_channel_pipes_add allows to add a single item
to all clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required only if
you are handling data in a polling loop (and thus, you are preventing
the default event loop from running).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display-channel.h contains lots of information used by different
DisplayChannel components.
In the past all RedWorker, CursorChannel and DisplayChannel code was in
a single file. Since lots of code to handle DisplayChannel is still in
RedWorker, display-channel.h contains a lot of declarations so that they
can be accessed from RedWorker.
Moving declarations that are not needed by RedWorker and other external
class components helps to reduce dependencies between RedWorker and
DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch allocates VMC IDs by finding the first ID not used
instead of using a global variable and incrementing the value
for each channel created.
This solves some potential issues:
- remove the global state potentially making possible
to use multiple SpiceServer on the same process;
- don't potentially overflow the variable. This can happen if
channels are allocated/deallocated multiple times
(currently not done by Qemu).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This can happen as the connection is asynchronous so (MT main thread,
CT channel thread):
- MT you get a new connection;
- MT a connection is sent to CT;
- MT you get a disconnection of main channel;
- MT red_client_destroy is called;
- CT you attempt to add the RCC to RedClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
A RedChannelClient is always attached to a valid RedChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Name will be visible in debugger and /proc filesystem
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The message is asynchronous so to avoid the object to potentially
been released before being processed keep a strong reference to
it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_disconnect_display() is duplicating what red_channel_disconnect()
already does, so red_disconnect_display() and red_disconnect_cursor()
are actually identical code-wise. We can directly call
red_channel_disconnect() from flush_commands() rather than passing a
'red_disconnect_t disconnect' argument to that function.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
cursor_channel_disconnect() calls
cursor_channel_client_reset_cursor_cache() on all CursorChannelClient
associated with the current CursorChannel before calling
red_channel_disconnect().
red_channel_disconnect() will iterate over all CursorChannelClient
calling red_channel_client_disconnect(), which will eventually call
CursorChannelClient::on_disconnect. This will in turn
cursor_channel_client_reset_cursor_cache(), so calling it in
cursor_channel_disconnect() before calling red_channel_disconnect() is
redundant.
cursor_channel_disconnect() can thus be replaced by a direct call to
red_channel_disconnect().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SoundChannelClient has a stub implementation of
RedChannelClient::on_disconnect(), this commit removes the need for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This vfunc only has a RedChannelClient * argument, and most of the time,
it operates on RedChannelClient, not on RedChannel. Moreover, the only
time it's used is from RedChannelClient. This commit moves the vfunc to
RedChannelClient, which seems like a better fit for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
ORC library is used internally by GStreamer to generate code
dynamically.
If ORC cannot allocate executable memory, the failure causes
an abort(3) to be called.
This happens on some SELinux configurations that disable executable
memory allocation (execmem boolean).
Check that ORC could work before attempting to use GStreamer to
avoid crashes.
While this check is done, the ORC library outputs an error which will
be well visible in Qemu output.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If you are testing for NULL data this means that variable could be
NULL so avoid to access before the check to make sure the check is hit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The multimedia time is defined as uint32_t.
Use the proper type instead of int.
Currently no arithmetic is done on this value but
just copies so considering that on the architectures
we support sizeof(int) == sizeof(uint32_t) there's
no change in the resulting machine code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
A RedClient can be freed from the main thread following a main channel
disconnection (reds_client_disconnect). This can happen while another
thread is allocating a new channel client for that client.
To prevent the usage of a pointer which can be invalid
take ownership of the pointer.
Note that we don't need this when disconnecting as disconnection is
done synchronously (the dispatch messages are registered with
DISPATCH_ACK).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Trace the number of loops done processing display commands
and the number of loops in which the queue was full.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
If a DisplayChannelClient cannot be instantiated capabilities
are not released correctly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display variable already contains the DCC_TO_DC(dcc) value so
reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Some RedChannelClient data members were marked as int when they only
hold booleans.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
reds_get_n_clients is a single line and is used only by
spice_server_get_num_clients.
The 2 functions have very similar names so inlining
reds_get_n_clients does not make code less readable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The leak detector we use currently is not enough to detect
some kind of leak in DisplayChannel so manually test.
These tests are enabled only when --enable-extra-checks is passed
to configure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Mostly of red_channel_destroy calls were preceded by
a call to unregister the channel.
The only exception was the main channel as this channel is
always present and its initialisation is a bit different.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
RedWorker should not handle directly to client but
defer the job to DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make easier to understant the value to use in the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
