RedChannel uses OutgoingHandlerInterface to provide constant pointers to
RedChannelClient methods. This OutgoingHandlerInterface structure is
then used in RedChannelClient to indirectly call these methods.
The OutgoingHandlerInterface abstraction is unused, ie the codebase
only has a single implementation for it, so we can directly call the
relevant methods and make them static instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <figlio@redhat.com>
Have the RedChannelClient callback call into a RedChannel callback
rather than doing the opposite. This will be useful in some subsequent
refactoring of this code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_parse() currently does roughly:
if (klass->parser) {
parsed = klass->parser(msg, msg_size, &parsed_size);
klass->handle_parsed(rcc, parsed_size, msg_type, parsed);
} else {
klass->handle_message(rcc, msg_type, msg, msg_size);
}
The handle_parsed implementation expects a void * 'parsed' argument,
which will then be cast to the correct type. There is not really a need
to provide distinct handle_parsed/handle_message vfuncs, instead we can
say that if a RedChannel subclass provides a 'parser' vfunc, then it's
'handle_message' vfunc will be called with the parsed message, otherwise
it will be called with unparsed data (ie what handle_message currently
expects).
This makes the code slightly easier to follow as messages will always be
handled by the same vfunc.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
smartcard_channel_client_pipe_add_push was just calling
red_channel_client_pipe_add_push without any cast or other
changes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This makes the code slightly more readable as this means less local
variables to keep track of when taking a high level view of that code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
read_binary() attaches 'mem' to the SpiceReplay::allocated list.
On failure, SpiceReplay::allocated and its content are freed by
spice_replay_free().
SpiceReplay::primary_mem is also freed, which causes a double free
as replay_handle_create_primary() added 'mem' both to
SpiceReplay::primary_mem and SpiceReplay::allocated.
This commit avoids this by ensuring SpiceReplay::primary_mem is not
kept in the SpiceReplay::allocated list.
Note that this double free can happen only on currupted or wrong
record images.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The Surface and Display channels each have a 'current_list' Ring, and
Surface also has a 'current' Ring. these names are confusing, so at
minimum, add a comment indicating the type of object they hold. The
DisplayChannel::current_list already had a comment, but it was
incorrect.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Caller is supposed the function return a buffer able to store
size bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limits for capabilities are specified using 32 bit unsigned integers.
This could cause possible integer overflows causing buffer overflows.
For instance the sum of num_common_caps and num_caps can be 0 avoiding
additional checks.
As the link message is now capped to 4096 and the capabilities are
contained in the link message limit the capabilities to 1024
(capabilities are expressed in number of uint32_t items).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limit for link message is specified using a 32 bit unsigned integer.
This could cause possible DoS due to excessive memory allocations and
some possible crashes.
For instance a value >= 2^31 causes a spice_assert to be triggered in
async_read_handler (reds-stream.c) due to an integer overflow at this
line:
int n = async->end - async->now;
This could be easily triggered with a program like
#!/usr/bin/env python
import socket
import time
from struct import pack
server = '127.0.0.1'
port = 5900
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect((server, port))
data = pack('<4sIII', 'REDQ', 2, 2, 0xaaaaaaaa)
s.send(data)
time.sleep(1)
without requiring any authentication (the same can be done
with TLS).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_handle_message can handle base messages
so reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Some gcc version reports this error:
stat-file.c: In function 'stat_file_add_node':
stat-file.c:180:15: error: 'node' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
g_strlcpy(node->name, name, sizeof(node->name));
^~~~
cc1: all warnings being treated as errors
This warning is a false positive as this loop:
for (ref = 0; ref <= stat_file->max_nodes; ref++) {
node = &stat_file->stat->nodes[ref];
...
}
will always iterate at least once.
This patch rewrite the loop in order to make more compilers
understand that node variable is always initialized.
There are two checks apparently removed in the patch:
- check for stat_file->stat not being NULL. This was worthless as the
field was already used in the function. Also this field is never
NULL (unless memory corruption happened);
- stat_file->stat->num_of_nodes >= stat_file->max_nodes. It's implicit
in the loop. If num_of_nodes >= max_nodes means that there are no
free nodes so all nodes should have SPICE_STAT_NODE_FLAG_ENABLED set,
loop will exit and function will return INVALID_STAT_REF.
Reported-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The stat file contains an array of max_nodes elements
so we must stay in [0, max_nodes) range, not [0, max_nodes].
There are no spice path that lead to these overflows but
it's better to have them fixed before creating one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Adding conditional for having gstreamer_0_10 or gstreamer_1_0,
removing the previous conditionals and update relevant ifdefs
with the newly defined conditional
We support only a single client so don't waste code just
to check this.
The worst stuff can happen is that we'll migrate multiple
connections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Move the freeing of SndChannel data members from snd_detach_common() to
the finalize function to encapsulate things a bit more cleanly. It
doesn't really change the behavior or order of destruction since
snd_detach_common() destroys the channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Sound implementation used internal RedChannelClient data while now
it just uses the public interface not thouching RedChannelClient
internal state so now is possible to make this field private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Now that SndChannelClient has switched from using its own code for
sending data to using RedChannelClient, it's very close to being an
actual RedChannelClient.
This commit makes it directly inherit from RedChannelClient rather than
having a channel_client field. This allows to get rid of the whole
DummyChannel/DummyChannelClient code.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
snd_set_command()/snd_send() are higher level methods which take care of
scheduling calls to the corresponding snd_*_send_*() methods when
appropriate. This commit switches a few direct snd_*_send_*() calls to
snd_set_command()/snd_send().
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
You can see that SndChannelClient has much less field
as the code to read/write from/to client is reused from
RedChannelClient instead of creating a fake RedChannelClient
just to make the system happy.
One of the different between the old sound code and all other
RedChannelClient objects was that the sound channel don't use
a queue while RedChannelClient use RedPipeItem object. This was
the main reason why RedChannelClient was not used. To implement
the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
will be queued to RedChannelClient (only one!) so signal code we
have data to send. The {playback,record}_channel_send_item will
then send the messages to the client using RedChannelClient functions.
For this reason snd_reset_send_data is replaced by a call to
red_channel_client_init_send_data and snd_begin_send_message is
replaced by red_channel_client_begin_send_message.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
The removed code was trying to read data when
spice_server_record_get_samples() is called. Since reading of data is
event-driven anyway (see snd_event), it's redundant to try
again to read more data.
This commit removes this code as this will some refactoring easier in
the next commits.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can get it from our DummyChannelClient rather than storing it in
SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can use the marshaller provided by the dummy RedChannelClient
associated with SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Filter out commands which should not happen. Should it be a
g_warn_if_fail() or such instead?
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is in preparation for switching SndChannelClient into a proper
RedChannelClient. The prototype of the new helper matches what is
expected from the RedChannel::config_socket vfunc.
To be able to achieve that, this commit associates the sound channel
RedsStream instance with the DummyChannelClient instance we have, and
then call snd_channel_config_socket() on that instance.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The main goal of this commit is to avoid to dereference 'client' before
it's checked for NULL. This meant splitting one error condition in 2
separate ones.
This also sets default values for the 'frame' and 'num-samples'
out parameters so that we just have to return on error conditions.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This structure is used to store format information for
both Gstreamer 0.10 and 1.0 however the two format uses
different fields from it.
Use a macro to filter only needed fields.
This currently also fixes a compile error using Gstreamer 0.10
(GST_VIDEO_FORMAT_RGB15 not defined as not available).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The structure is used only to allocate private data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently, calling spice_server_remove_interface() twice in a row with
the same SPICE_INTERFACE_CHAR_DEVICE is going to cause a crash when
calling red_char_device_get_server(char_device->st); because
char_device->st will have been set to NULL by the first call.
This commit adds a few sanity checks before trying to use the various
'st' members of the interfaces.
This should avoid the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=1411194 even though it's not
clear how we got in that situation.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid to use g_object_get if not necessary.
red_char_device_get_server is more type safe and we are
not bound to dynamic fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This field is used only for debugging.
Remove it reducing a bit all these "current" fields around.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If bitmap stride and stream stride are different copy was used.
Using GStreamer 1.0 you can avoid the copy setting correctly
image offset and stride.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Handling read returning 0 (usually end of connection/pipe)
is the same of handling an error (read result -1) with errno == 0
so merge the two paths to reuse code and simplify.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.
This resolves rhbz#1416692.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Tested-by: Daniel P. Berrange <berrange@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
