If worker->qxl->id is bigger than 0x7ffffff (in other words, it's a
negative signed int) then
printf(worker_str, "display[%d]", worker->qxl->id);
will need:
"display[]" -> 9 bytes
%d -> 11 bytes
The trailing \0 will thus overflow our 20 bytes destination.
As QXLInstance::id should be an unsigned int, this commit changes the
format string to use %u. This also switches to snprintf.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
No reason to pass through MainDispatcher, the purpose of
MainDispatcher is to call reds functions from the right thread.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
To avoid potential regressions, check it only if extra checks are
enabled.
This allows to check previous "Move channel registration to constructed
vfunc" commit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
We should not reuse the same interface twice as doing so will
cause dangling pointers.
Unregister it at every iteration.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
I experienced some crashes with qemu 3.1.0 compiled with libspice-server
0.14.0 on Gentoo.
The problem reproduced reliably with a guest running Ubuntu 18.04.2 LTS.
If I connect a viewer at system startup, I would get a crash just after
the fade-in of the login prompt in GDM.
Interestingly, I usually was unable to reproduce the issue if I waited
to connect until after the greeter was fully displayed.
The patch I used to correct the issue for me applies to the master
branch cleanly, so I suspect the problem may still exist.
The only other references to this issue I could find were two abrt
reports in CentOS:
https://bugs.centos.org/view.php?id=15171https://bugs.centos.org/view.php?id=15441
I'm not sure if the agent->video_encoder is supposed to be guaranteed to
exist when this function is called.
Signed-off-by: Douglas Paul <doug@bogon.ca>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
For the Display Channel and the Cursor channel, move the call to
reds_register_channel() to the _constructed() vfunc rather than calling
it explicitly from RedWorker. This matches what other channels do.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The difference is subtle but compared to what client receives, this
could help identify values set to the wrong head, e.g:
First we received:
| display-channel.c:180:monitors_config_debug: monitors config count:2 max:4
| display-channel.c:184:monitors_config_debug: +0+0 1015x805
| display-channel.c:184:monitors_config_debug: +1015+0 1024x740
And then:
| display-channel.c:180:monitors_config_debug: monitors config count:3 max:4
| display-channel.c:184:monitors_config_debug: +0+0 1015x805
| display-channel.c:184:monitors_config_debug: +0+0 0x0
| display-channel.c:184:monitors_config_debug: +1015+0 1024x740
In the first debug it would be helpful to have "head 0" and "head 1",
to point out the temporary error in monitor's config message.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
For coding style consistency, use 'bool' when we want to represent a
boolean value.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Only send the graphics device display info to agents that advertise the
VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Make this a RedsState member function rather than a standalone function.
This means that we simply pass RedsState* as an argument rather than the
internal member variables of RedsState. This enables the following
commit which handles the VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability to
avoid sending graphics device info to agents that do not support it.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
usleep under Windows does not seem to have the required precision.
Use milliseconds and adjust check times according.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Formatting string should be compatible with GLib.
GLib uses formatting types compatible with GNU.
For Linux this is not an issue as both systems (like a printf) and
GLib one uses the same formatting type. However on Windows they
differs potentially causing issues.
This is also make worse as GLib 2.58 changed format attribute from
__printf__ to gnu_printf (Microsoft compatibility formats like %I64d
are still supported but you'll get warnings using GCC/Clang
compilers).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
RedPipeItem is not using the Ring structures anymore.
Also is not using GLib functionality.
Just include base headers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid to register multiple interface with the same ID.
This would result in issues as 2 channels would have the same
(channel_type, channel_id) which must be unique.
Qemu always allocates QXL interface with IDs starting from 0.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The field is only assigned in do_spice_init, surely won't change
in the meanwhile.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This had already been done for autotools in spice-common commit
924f47a653bd87fbd50229ee34b58d7b9a3f1ec8.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This was added in bd8771adbc.
There's no reason to not use reds function instead.
MainDispatcher needs to listen in the main thread that is the
one provided by reds_core_* functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of scanning the monitor twice (one to compute the size
and another to build the message) use a single function to
marshal the message.
This also fixes big endian machines (which are not supported).
Marshal function is exported to make easier to test (see following
patch).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Most of the time result is set to AGENT_MSG_FILTER_OK, set at
the beginning and change if necessary.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This finished the work of 90ff154b36
(cfr "Update usage of GObject private structures").
Removes last call to g_type_class_add_private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
This removes a not really useful switch/case, and changes the function
to exit early on error cases, rather than exiting early in the nominal
case.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This renames the method to smartcard_new_vsc_msg_item as this creates a
new object. This also removes the creation of a temporary VHeader which
is then going to be duplicated.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The function returns NULL if vheader->type is VSC_Init so no need to
check it a second time.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Only RedCharDeviceVDIPortClass implements this vfunc, rather than
forcing every classes deriving from RedCharDeviceClass to implement it,
red_char_device_send_tokens_to_client() can deal with it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
New functions and macros have been added in glib 2.38 to better handle
this case.
c8de2b11bb/NEWS
G_TYPE_INSTANCE_GET_PRIVATE will be deprecated in GLib 2.58.
https://gitlab.gnome.org/GNOME/glib/merge_requests/7/commits
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Bugs such as https://bugzilla.redhat.com/show_bug.cgi?id=1651882 can be
quite tricky to figure out without the detailed OpenSSL error. This
commit adds a detailed dump of the OpenSSL error stack when an OpenSSL
failure happens.
In the bug above, this would have displayed:
(process:13154): Spice-WARNING **: 05:43:10.139: reds.c:2816:reds_init_ssl: Could not load certificates from /etc/pki/libvirt-spice/server-cert.pem
(process:13154): Spice-WARNING **: 05:43:10.140: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
RedMemSlotInfo keeps an array of groups, and each group contains an
array of slots. Unfortunately, these checks are off by 1, they check
that the index is greater or equal to the number of elements in the
array, while these arrays are 0 based. The check should only check for
strictly greater than the number of elements.
For the group array, this is not a big issue, as these memslot groups
are created by spice-server users (eg QEMU), and the group ids used to
index that array are also generated by the spice-server user, so it
should not be possible for the guest to set them to arbitrary values.
The slot id is more problematic, as it's calculated from a QXLPHYSICAL
address, and such addresses are usually set by the guest QXL driver, so
the guest can set these to arbitrary values, including malicious values,
which are probably easy to build from the guest PCI configuration.
This patch fixes the arrays bound check, and adds a test case for this.
This fixes CVE-2019-3813.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There should be no reason for the caller to modify the internal
structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Client process termination did not work for Windows, used Win32
APIs.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Windows does not support Unix sockets.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
MingW does not include this header while including stdint.h so
on Windows you need to include it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
This is a preparatory patch for next portability patches
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
display-channel.h is not using any of the definition from
these headers.
Adding a missing include to red-worker.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid using strncpy, considered not secure.
In this case a simple memcpy is used, we are going to terminate
the string in any case on the next line.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Lukáš Hrázký <lhrazky@redhat.com>
Zlib structure take up more than 1MB and it is rarely used nowadays
as it is not much effective.
Initialise it only when necessary saving some memory in the normal
case.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When a client disconnects, smartcard_channel_client_set_char_device
is called with a NULL "device" argument. In that case there is
no need to take a weak reference to the device.
Without this patch the server complains:
g_object_add_weak_pointer: assertion 'G_IS_OBJECT (object)' failed
and aborts when a second client attempts to connect.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
