There is exactly one user in RedChannel, and this can be reimplemented
using already public RedChannelClient API. No need for an extra
function very specialized function with a not great name.
This commit thus removes one method from RedChannelClient public API,
and replaces it with an equivalent private helper in RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
red_channel_disconnect_if_pending_send() and red_channel_wait_all_sent() are
always called together, we can remove one of the 2 methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In case GLib don't provide these functions we use replacements so
there's no need to have a warning if these functions are called.
This potentially capture other compatibility issues in the tests
that would be ignored having all deprecation warnings disabled.
Tested with GLib 2.28 and 2.52.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When the worker is started it could take a while to start processing
commands.
The reason is that the dispatcher handler is called after the worker
so GLib will receive a FALSE answer to both prepare and check
callbacks of the RedWorkerSource causing GLib to wait till another
event is received.
This is a regression since the introduction of GLib event loop, before
the command processing was always attempted after any events.
Commands (from QXL interface for cursor and display) are processed
during the RedWorkerSource dispatch so if they are not processed just
when the VM is started they will be processed on next event which
could be from dispatcher (main thread requests), from existing
connections or from pending timers. However in the case there are no
clients connected and no other requests from main thread the worker
thread won't process them.
Setting the event_timeout to 0 cause the prepare callback to return
TRUE so GLib will dispatch the RedWorkerSource.
This was discovered attempting to use the tests in server/tests
directory to reproduce a leak in RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Pipelines are never freed.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Command line options are not freed at the end of the program.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
encoder_name is never NULL as already initialized with "mjpeg" value.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If a client is unable to complete the TLS handshake phase
reds_init_client_ssl_connection leaked some memory as the stream is not
correctly freed.
This also causes the stream to send the SPICE_CHANNEL_EVENT_DISCONNECTED
event. Otherwise only SPICE_CHANNEL_EVENT_CONNECTED was sent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently is possible to trigger a leak by passing an invalid
connection.
This can happen if the client opens a connection and then closes it
without writing or reading any data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The code tests for the presence of RedChannelClient::stream while
initializing RedChannelClient.
However, the check was done too late, and a
RedChannelClient::config_socket implementation (for example
snd_channel_client_config_socket) could have tried to use it before the
check that it's not NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This potentially can also save the copy if there is enough
space to resize the buffer in place.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
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>
