The self->error is the error set for the file-transfer and it will be
propagated with the "finish" signal. As this is transfer none pointer,
we should not lose its reference on g_task_return_error and we should
clear it out afterwards.
40 (16 direct, 24 indirect) bytes in 1 blocks are definitely lost in
loss record 7,489 of 14,298
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0xB5090E8: g_malloc (gmem.c:94)
by 0xB51F8A2: g_slice_alloc (gslice.c:1025)
by 0xB4EFCC5: g_error_new_literal (gerror.c:471)
by 0xB4EFFAD: g_set_error_literal (gerror.c:619)
by 0xAF13397: g_cancellable_set_error_if_cancelled (gcancellable.c:314)
by 0xAF630C8: g_task_propagate_error (gtask.c:1519)
by 0xAF63CD8: g_task_propagate_int (gtask.c:1652)
by 0x50863F5: spice_file_transfer_task_read_finish (spice-file-transfer-task.c:477)
by 0x5093239: file_xfer_read_async_cb (channel-main.c:1811)
by 0xAF62F38: g_task_return_now (gtask.c:1121)
by 0xAF63775: g_task_return (gtask.c:1179)
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
g_file_read_finish() is (transfer full) so we must release the ref
we got in _dispose() as it's not done anywhere else in the code.
Acked-by: Victor Toso <victortoso@redhat.com>
The file transfer code calls g_*_async() methods with a GTask as the
user_data argument. It needs to own a reference to the GTask for the
duration of the async call to keep the GTask alive, however it was never
releasing it when it no longer needs it (that is, after calling
g_task_return_*).
Some of the leaks fixed by this patch are:
* GTask leak after flush
11,232 bytes in 54 blocks are definitely lost in
loss record 14,018 of 14,071
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0xC14A0E8: g_malloc (gmem.c:94)
by 0xC1608A2: g_slice_alloc (gslice.c:1025)
by 0xC160ECD: g_slice_alloc0 (gslice.c:1051)
by 0xBEDBD11: g_type_create_instance (gtype.c:1857)
by 0xBEBD7DA: g_object_new_internal (gobject.c:1781)
by 0xBEBF11C: g_object_newv (gobject.c:1928)
by 0xBEBF89B: g_object_new (gobject.c:1621)
by 0xBBA4424: g_task_new (gtask.c:693)
by 0x6F8C3C0: file_xfer_flush_async (channel-main.c:899)
by 0x6F8C3C0: file_xfer_read_async_cb (channel-main.c:1826)
by 0xBBA3F38: g_task_return_now (gtask.c:1121)
by 0xBBA4775: g_task_return (gtask.c:1179)
* GTask leak from spice_main_file_copy_async()
208 bytes in 1 blocks are definitely lost in
loss record 13,708 of 15,093
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0xC14A0E8: g_malloc (gmem.c:94)
by 0xC1608A2: g_slice_alloc (gslice.c:1025)
by 0xC160ECD: g_slice_alloc0 (gslice.c:1051)
by 0xBEDBD11: g_type_create_instance (gtype.c:1857)
by 0xBEBD7DA: g_object_new_internal (gobject.c:1781)
by 0xBEBF11C: g_object_newv (gobject.c:1928)
by 0xBEBF89B: g_object_new (gobject.c:1621)
by 0xBBA4424: g_task_new (gtask.c:693)
by 0x6F8EF55: spice_main_file_copy_async (channel-main.c:3084)
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The GFileInfo returned by spice_file_transfer_task_init_task_finish()
must be unref'ed when no longer needed.
Acked-by: Victor Toso <victortoso@redhat.com>
This is here due bad rebase. It was removed and replaced by patch
f6072e678e but not removed from 1af22e9652 "file-xfer:
move to spice-file-transfer-task.c", so the function is there, unused.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
We own a reference on the GAsyncResult returned by
g_task_propage_pointer() so we have to g_object_unref() it when we no
longer need it.
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
This bogus code was introduced when switching to GTask API. Seems that
while writing those patches I just overlooked this part and set the wrong
result for the task. As part of the problems introduced (and now fixed)
you can notice that no output stream was being sent to the guest.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Create it on SPICE_PORT_EVENT_OPENED
From g_cancellable_reset documentation:
Note that it is generally not a good idea to reuse an existing
cancellable for more operations after it has been cancelled once,
as this function might tempt you to do. The recommended practice is
to drop the reference to a cancellable after cancelling it, and let it
die with the outstanding async operations. You should create a fresh
cancellable for further async operations
In our case reusing the cancellable leads to a crash:
#0 0x00007ffff1662940 in g_task_return_error () at /lib64/libgio-2.0.so.0
#1 0x00007ffff1662b60 in g_task_return_new_error () at /lib64/libgio-2.0.so.0
#2 0x00007ffff57916e0 in read_cancelled (cancellable=<optimized out>, user_data=<optimized out>) at vmcstream.c:182
#3 0x00007ffff1391555 in g_closure_invoke () at /lib64/libgobject-2.0.so.0
#4 0x00007ffff13a4062 in signal_emit_unlocked_R () at /lib64/libgobject-2.0.so.0
#5 0x00007ffff13acf5f in g_signal_emit_valist () at /lib64/libgobject-2.0.so.0
#6 0x00007ffff13ad33f in g_signal_emit () at /lib64/libgobject-2.0.so.0
#7 0x00007ffff16100d8 in g_cancellable_cancel () at /lib64/libgio-2.0.so.0
#8 0x00007ffff5775c01 in port_event (self=0xc7af60, event=1) at channel-webdav.c:512
#9 0x00007ffff1391555 in g_closure_invoke () at /lib64/libgobject-2.0.so.0
#10 0x00007ffff13a445d in signal_emit_unlocked_R () at /lib64/libgobject-2.0.so.0
#11 0x00007ffff13acf5f in g_signal_emit_valist () at /lib64/libgobject-2.0.so.0
#12 0x00007ffff577479a in emit_main_context (opaque=0x7fffa5fff8d0) at gio-coroutine.c:200
#13 0x00007ffff10b7847 in g_idle_dispatch () at /lib64/libglib-2.0.so.0
#14 0x00007ffff10bade2 in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#15 0x00007ffff10bb160 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
#16 0x00007ffff10bb20c in g_main_context_iteration () at /lib64/libglib-2.0.so.0
#17 0x00007ffff1677d2d in g_application_run () at /lib64/libgio-2.0.so.0
#18 0x000000000041031a in main (argc=2, argv=0x7fffffffd928) at remote-viewer-main.c:42
Fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=97113
When using GtkDrawingArea and EGL was not used, it was still initialized.
This produced warning messages on systems where EGL is not supported.
Move spice_egl_init from drawing_area_realize to gl_scanout.
Fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=95254
While reviewing lz4 patch, I found that there are potential leaks. This
is untested, fyi. It would be nice if someone could confirm with running
ASAN for example.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Snir Sheriber <ssheribe@redhat.com>
Windows does not like Pause key sent with same scancodes as Break.
Although is the same physical key the two functions send two completely
different set of codes. On key press a E1 1D 45 sequence is generated
while on key release a E1 9D C5 is generated. Also some hardware
keyboards send press+release at the same time on key press (nothing on
release).
Tested with Linux and Windows clients.
Tested with Linux, Windows and DOS guests.
On Windows guest VK_PAUSE was not arriving correctly.
An additional send_pause function was added to avoid to change the
normal flow too much. This as the keymap table (generated from
src/keymaps.csv) can hold only one scancode while this key generate two
scancodes (ie E1-1D and 45) for each event.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Not only E0 prefix but also E1 and E2.
They are used in some special cases, one is the pause key.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
'area' was being set within the 'else' branch, but was not actually
used. So just move the declaration of 'area' within the if statement
where it is actually used. This potentially avoids "set-but-not-used"
warnings when GDK_WINDOWING_X11 is not defined.
This fixes the build when done out of the tree, because of recent commit
in spice-common with auto-generated files.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Compressed message type is CompressedData which contains compression
type (1 byte) followed by the uncompressed data size (4 bytes-exists
only if data was compressed) followed by the compressed data
If SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4 capability is available &&
data_size > COMPRESS_THRESHOLD && !AF_LOCAL data will be sent
compressed otherwise data will be sent uncompressed (as well if
compression has failed)
Update required spice-protocol to 0.12.12
Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Thanks to ASAN, I found this off-by-one memory access in the unix2dos
code:
/util/unix2dos: =================================================================
==23589==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000dd2f at pc 0x00000040428e bp 0x7ffd6fc31b90 sp 0x7ffd6fc31b80
READ of size 1 at 0x60200000dd2f thread T0
#0 0x40428d in spice_convert_newlines /home/elmarco/src/spice/spice-gtk/src/spice-util.c:355
#1 0x40443a in spice_unix2dos /home/elmarco/src/spice/spice-gtk/src/spice-util.c:382
#2 0x401eae in test_unix2dos /home/elmarco/src/spice/spice-gtk/tests/util.c:69
#3 0x7fb8bcd81983 (/lib64/libglib-2.0.so.0+0x6e983)
#4 0x7fb8bcd81b4e (/lib64/libglib-2.0.so.0+0x6eb4e)
#5 0x7fb8bcd81d5d in g_test_run_suite (/lib64/libglib-2.0.so.0+0x6ed5d)
#6 0x7fb8bcd81d80 in g_test_run (/lib64/libglib-2.0.so.0+0x6ed80)
#7 0x402cce in main /home/elmarco/src/spice/spice-gtk/tests/util.c:207
#8 0x7fb8bc755730 in __libc_start_main (/lib64/libc.so.6+0x20730)
#9 0x401818 in _start (/home/elmarco/src/spice/spice-gtk/tests/util+0x401818)
0x60200000dd2f is located 1 bytes to the left of 4-byte region [0x60200000dd30,0x60200000dd34)
allocated by thread T0 here:
#0 0x7fb8c10421d0 in realloc (/lib64/libasan.so.3+0xc71d0)
#1 0x7fb8bcd61f1f in g_realloc (/lib64/libglib-2.0.so.0+0x4ef1f)
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/elmarco/src/spice/spice-gtk/src/spice-util.c:355 in spice_convert_newlines
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Fix gtk-doc warning by ignoring the new private header.
./spice-gtk-unused.txt:1: warning: 12 unused declarations.They should be
added to spice-gtk-sections.txt in the appropriate place.
While at it, fix grabsequence header incorrect file name.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Agent only can only send error or cancel from a transfer operation
after it was initialized. From SpiceFileTransferTask point of view,
error and cancellation is an GError with different code so testing
only cancel seems enough.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This only includes a simple test for file-transfer with a small
summary of the possible situations of the test.
As the test is specifically for SpiceFileTransferTask, we don't create
a SpiceMainChannel.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch moves:
* GObject boilerplate
* External API related to SpiceFileTransferTask
* Internal API needed by channel-main
* Helpers that belong to this object
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Another patch to rename variable and keep consistency across
channel-main. Let's avoid task which is common for GTask and self for
non SpiceMainChannel objects
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This debug information is simple way to check that the file-transfer
is not stalled. Now that we are using the read-async functions, we can
move this debug info there and avoid accessing object internals in
channel-main.c
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Rename:
* file_xfer_close_cb -> spice_file_transfer_task_close_stream_cb
As this will be private to SpiceFileTransferTask
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
SpiceFileTransferTask has a callback to be called when operation
ended. Til this patch, we were setting the user callback which means
that in multiple file-transfers, we were calling the user callback
several times.
Following the same logic pointed from 113093dd00 this
is a SpiceMainChannel operation and it should only call the user
callback when this operation is over (FileTransferOperation now).
Highlights:
* Now using GTask to set user callback and callback_data
* Handling error on FileTransferOperation:
- It is considered an error if no file was successfully transferred due
cancellations or any other kind of error;
- If any file failed due any error, we will consider the operation a
failure even if other files succeed.
Note also that SpiceFileTransferTask now does not have a callback and
callback_data. The xfer_tasks has ended after its "finish" signal is
emitted.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Before this patch, the progress_callback is being called from
SpiceFileTransferTask each time that some data is flushed to the agent
of this given xfer-task. The progress value is computed by iterating
on all available xfer-tasks.
This patch intend to fix/change:
* The progress_callback should be called only with information related
to the FileTransferOperation of given xfer-task; This was also
suggested by 113093dd00a1cf10f6d3c3589b7589a184cec081;
* In case a SpiceFileTransferTask is cancelled or has an error, we
remove the remaining bytes (not sent) of this file from the
transfer_size;
* The progress_callback should not be done from SpiceFileTransferTask
context by (proposed) design. As the transfer operation starts in
spice_main_file_copy_async(), FileTransferOperation should handle
these calls.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
No need to inform of a problem under
spice_file_transfer_task_completed() as the task will be finalized and
we can send the error to the agent there.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Each call to spice_main_file_copy_async will now create a
FileTransferOperation which groups all SpiceFileTransferTasks of the
copy operation in a GHashTable.
To ease the review process, this first patch introduces the structure
and organize the logic around the four following helpers:
* spice_main_channel_reset_all_xfer_operations
* spice_main_channel_find_xfer_task_by_task_id
* file_transfer_operation_free
* file_transfer_operation_task_finished
The _task_finished function is the new callback for "finished" signal
from SpiceFileTransferTask.
The file_xfer_tasks GHashTable is now mapped as:
(key) SpiceFileTransferTask ID -> (value) FileTransferOperation
This patch leaves a FIXME regarding progress_callback which will be
addressed in a later patch in this series.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is an intermediary step for the following patches to make the
changes more clear and easy to follow.
In file_xfer_send_progress(), I've renamed the variable self to
xfer_task as this is not SpiceFileTransferTask function.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch changes:
* rename function: file_xfer_queue -> file_xfer_queue_msg_to_agent
As it makes more clear what this helper function does;
* Use buffer provided by spice_file_transfer_task_read_finish()
instead of accessing SpiceFileTransferTask's private structure
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The errors related to SpiceFileTransferTask can be present:
* before the flush happens, on spice_file_transfer_task_read_async()
* during the async flush:
- cancel from user which is handled by file_xfer_flush_async
- cancel/error from agent, handled on main_agent_handle_xfer_status()
file_xfer_flush_finish() should not check internal errors of
SpiceFileTransferTask and only be worried about errors regarding the
flush itself.
Note that with or without this patch, in case of errors from agent, we
don't stop the flushing; it is only cancelled from user cancellation.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
By separating SpiceFileTransferTask from channel-main, we could choose
where to put the handler for messages. With the approach based on
spice_file_transfer_task_read_async(), we cannot have it under
spice-file-transfer-task.c due the need of callback and userdata on
_read_async.
It is much easier to keep this in channel-main and do not move any
VDAgent.
This patch reverts 349a52ca2d changes
but it does rename:
- function file_xfer_handle_status -> main_agent_handle_xfer_status
- variables task to xfer_task
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
* on VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA, if the file-transfer is
on pending state, spice_file_transfer_task_read_async() will call the
callback with error set.
* on VD_AGENT_FILE_XFER_STATUS_SUCCESS, if the file-transfer is on
pending state, spice_file_transfer_task_completed() will set the
error for this task.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Introduced functions (private):
* void spice_file_transfer_task_read_async()
* gssize spice_file_transfer_task_read_finish()
For a better abstraction of how to read from SpiceFileTransferTask and
handle its data, following the design of other objects like GFile and
GInputStream.
Due to the logic changes involved, some functions were created or
renamed to better address or match its place and purpose:
* spice_file_transfer_task_read_stream_cb
Callback for the actual read from GInpustStream; This is handling
the SpiceFileTransferTask bits only;
* file_xfer_read_cb -> file_xfer_read_async_cb
Renamed to match _read_async() function; This is handling the data
from reading the file by flushing it to the agent.
As the _read_async() uses GTask, the error handling is done on the
channel-main's callback, after _read_finish() is called.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Introduced functions (private):
* void spice_file_transfer_task_init_task_async()
* GFileInfo *spice_file_transfer_task_init_task_finish()
The init process of SpiceFileTransferTask does initialize its
GFileInputStream (for reading the file) and also its GFileInfo
(necessary to protocol in order to start the file-transfer with agent)
Due the logic changed involved, some functions were renamed to better
match its place and purpose:
* file_xfer_info_async_cb -> file_xfer_init_task_async_cb
It is channel-main's callback for each _init_task_async()
* file_xfer_read_async_cb -> spice_file_transfer_task_read_file_cb
* file_xfer_info_async_cb -> spice_file_transfer_task_query_info_cb
Both should be private to SpiceFileTransferTask now.
As the _init_task_async() uses GTask, some error handling was moved to
channel-main's after _init_task_finish() is called.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
We can split from file_xfer_send_start_msg_async() the logic in
creating the SpiceFileTransferTasks; The rest of the function can be
handled at spice_main_file_copy_async().
The new function, spice_file_transfer_task_create_tasks() returns a
GHashTable to optimize the access to a SpiceFileTransferTask from its
task-id, which is what we receive from the agent.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
In order for channel-main to interact with each SpiceFileTransferTask
for the file-transfer operation, the following functions are
introduced:
* spice_file_transfer_task_get_id
* spice_file_transfer_task_get_channel
* spice_file_transfer_task_get_cancellable
Note that "id" property is public and could be acquired by
g_object_get but having the helper function is more practical.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is a follow-up of the previous commit ('usb-channel: Really stop
listening for USB events on disconnection').
Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE even if
spice_usb_device_manager_stop_event_listening() was not enough. When
this happens, this means that there is a bug in the internal users of
spice_usb_device_manager_start_event_listening(), but with this change,
we'll at least warn about it, and avoid a thread leak/potential future
crash.
When using USB redirection, it's fairly easy to leak the thread handling
USB events, which will eventually cause problems in long lived apps.
In particular, in virt-manager, one can:
- start a VM
- connect to it with SPICE
- open the USB redirection window
- redirect a device
- close the SPICE window
-> the SpiceUsbDeviceManager instance will be destroyed (including the
USB context it owns), but the associated event thread will keep running.
Since it's running a loop blocking on libusb_handle_events(priv->context),
the loop will eventually try to use the USB context we just destroyed
causing a crash.
We can get in this situation when redirecting a USB device because we
will call spice_usb_device_manager_start_event_listening() in
spice_usbredir_channel_open_device(). The matching
spice_usb_device_manager_stop_event_listening() call is supposed to
happen in spice_usbredir_channel_disconnect_device(), however by the
time it's called in the scenario described above, the session associated
with the channel will already have been set to NULL in
spice_session_channel_destroy().
The session is only needed in order to get the SpiceUsbDeviceManager
instance we need to call spice_usb_device_manager_stop_event_listening()
on. This patch stores it in SpiceChannelUsbredir instead, this way we
don't need SpiceChannel::session to be non-NULL during device
disconnection.
This should fix the issues described in
https://bugzilla.redhat.com/show_bug.cgi?id=1217202
(virt-manager) and most
likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes)
as well.
spice_usbredir_channel_disconnect_device_async() creates a GTask and
then calls g_task_run_in_thread(). This method will take the reference
it needs on the GTask, it does not take ownership of the passed-in
GTask. This means we need to unref the GTask we created after calling
g_task_run_in_thread(), otherwise we are going to leak the GTask, as
well as the channel it's associated with.
Since it's an USB redir channel, this also causes some USB device
manager/USB event thread leaks.
