This field was used only by RedChannelClient to mark when the socket
was shutdown. This condition can simply be tested by RedChannelClient
checking if there's a watch as is the only condition (beside object
destroying/disconnecting) where the watch is removed.
In any case the shutdown was used to understand if there were possible
data still to read.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedQxl and RedWorker are quite bound together running
CursorChannel and DisplayChannel in a separate thread
marshalling (RedQxl) and unmarshalling and executing
(RedWorker) requests.
Make the communication between them private trying
to facilitate maintaining these two files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
For dispatcher_register_handler(), use 'false' instead of 0 since the
last argument is a bool type now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
This callback was only executed for message types that were registered
with DISPATCHER_ASYNC ack type. However, the async_done handler was
called immediately after the message-specific handler and was called in
the same thread, so the async_done stuff can just as easily be done from
within the message-specific handler. This allows to simplify the
dispatcher_register_handler() method to simply require a boolean
argument for whether the message type requires an ACK or not.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Pass proper type to callback to avoid having to convert to
the right type for each call.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Is supposed to be used during initialization but is never
used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
AsyncCommand is used to handle asynchronous messages from the
dispatcher.
GL_DRAW_ASYNC is mainly using it to store the cookie.
The value of GL_DRAW_COOKIE_INVALID was choosen to allow implementing
cookies (which basically are handles) either using indexes (where 0 is
valid) or pointers (where 0 is invalid). Currently Qemu uses pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These 2 functions were doing the same stuff, calling
client_monitors_config callback in QXLInterface.
The only difference was that red_qxl_use_client_monitors_config
used a NULL value.
Added the check for proper version, QXLInstance before 3.3
did not have this callback.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@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);
which cause a crash like
(process:28742): Spice-WARNING **: Failed to create channel client: Client 0x40246f5d0: duplicate channel type 2 id 0
2017-08-24 09:36:57.451+0000: shutting down, reason=crashed
RedChannelClient is an GInitable type, which means that the object is
constructed, and then the _init() function is called, which can fail.
If the _init() fails, the newly-created object will be destroyed. As
part of _init(), we add a new watch for the stream using the core
interface that is associated with the channel. After adding the watch,
our rcc creation fails (due to duplicate ID), and the rcc object is
unreffed. This results in a call to reds_stream_free() (since the rcc
now owns the stream). But in reds_stream_free, we were trying to remove
the watch from the core interface associated with the RedsState. For
most channels, these two core interfaces are equivalent. But for the
Display and Cursor channels, it is the core Glib-based interface
associated with the RedWorker.
The watch in RedsStream by default is bound to the Qemu provided
SpiceCoreInterface while RedChannelClient bound it to Glib one causing
the crash when the watch is deleted from RedsStream. Change the bound
interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a stream is moved from the main thread to a
secondary one the events are potentially registered
using a different core interface. This cause memory
corruption accessing the watch registered in RedsStream.
This patch allows to always use the right interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This test runs 2 spice server in one program.
Use two different tcp port to be able to connect to both servers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add some check that something happened during creation/destruction.
Set as running on "make check".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Update tests names.
Remove tetris comments, never available and not planned.
Update some notes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
There's no need to not compile this feature, it just enable
a parameters which must be passed in order to change test
behaviour.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows to end the loop to end some tests.
Currently different tests enter the loop but never exit from
them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In C the sizeof(long) can be different than sizeof(void*),
use proper type.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not use calloc and malloc directly without checking
the result. Use instead spice functions to get a nice
error in case of allocation failures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
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>
