This is a revert of 93b4f4050^ and 93b4f4050.
For a SpiceCharDeviceInstance, the base interface must be a
SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
vmc_state/vmc_write/vmc_read implementations also have to be provided.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use new structures and functions to implement the statistics code.
Use inline functions instead of macros for increased type-safety.
If statistics are disabled, the structures and functions become
empty. This confines the configuration-specific #defines to the
statistics implementation itself and avoids the need for #defines in
the calling functions. This greatly reduces the chance of accidentally
breaking the build for one configuration or the other. The reds option
was removed from stat_inc_counter() as it was not used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Non blocking flag is set for all connection inside reds.c so
there's no need to set again for the single client channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not make it assume vec contains IOV_MAX elements.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
They no longer contain any vfuncs, so calling them "handler" does not
make a lot of sense. This commit renames them to
OutgoingMessageBuffer/IncomingMessageBuffer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Nothing outside of RedChannelClient needs access to data contained in
RedChannelClientPrivate, so we can move all the type definitions to the
.c file to make it fully opaque rather than relying on a private header.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The methods previously used by OutgoingHandlerInterface and
IncomingHandlerInterface are no longer used as generic callbacks,
but are directly called from RedChannelClient code. We can be explicit
about the type of their first argument (RedChannelClient *) rather than
using a generic void * pointer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of OutgoingHandler which relies
OutgoingHandler::opaque being a RedChannelClient. This commit makes this
explicit in order to get rid of the OutgoingHandler::opaque data member.
This renames red_peer_handle_outgoing() to
red_channel_client_handle_outgoing() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_outgoing() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_outgoing() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of IncomingHandler which relies
IncomingHandler::opaque to be a RedChannlClient. This commit makes this
explicit. This allows to get rid of the IncomingHandler::opaque data
member.
This renames red_peer_handle_incoming() to
red_channel_client_handle_incoming() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_incoming() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_incoming() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This commit removes what remains of IncomingHandlerInterface. The
remaining function pointers were pointing to RedChannel vfuncs.
Moreover the IncomingHandlerInterface 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 <fziglio@redhat.com>
This commit removes IncomingHandlerInterface::on_error and
IncomingHandlerInterface::on_input. As with previous commits, these
vfuncs are always calling the method, and RedChannel::init sets them to
point to RedChannelClient methods, which RedChannelClient is then going
to call indirectly through the IncomingHandlerInterface instance.
This commit changes this to direct calls to the corresponding methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Similarly to the previous commits, this removes an indirection level,
IncomingHandlerInterface stores pointers to
alloc_recv_buf/release_recv_buf vfuncs, but these are always set to
RedChannel::alloc_recv_buf/RedChannel::release_recv_buf, which are also
vfuncs which can be overridden if needed. This commit removes the
indirection and directly calls the relevant methods.
These vfuncs really should be in RedChannelClient rather than
RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
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>
