This commit resolves two distinct race conditions related to `QProcess`
lifecycle management in the VirtualMonitorPlugin.
First, a use-after-free could occur if `stop()` was called externally
(e.g., via a network packet or D-Bus) while `requestRdp()` was
executing. The function would assign a new `QProcess` to `m_process` and
then proceed to configure it. An intervening call to `stop()` would
delete the process, leaving `requestRdp()` operating on a dangling
pointer, which would lead to a crash.
Second, a data race existed in the `QProcess::finished` signal handler.
The lambda captured `this` and accessed the shared `m_process` member.
If a process crashed and the retry logic was triggered, `requestRdp()`
would be called again from within the lambda, reassigning `m_process` to
a new instance. The original lambda would then incorrectly operate on
the new process, for example by reading its error stream or managing its
lifecycle.
Both race conditions are fixed by changing how the `QProcess` object is
created and managed:
1. The `QProcess` is now created and fully configured in a local
variable. It is only assigned to the `m_process` member immediately
before being started. This minimizes the time window for the
use-after-free vulnerability.
2. The lambda connected to the `finished` signal now captures the
pointer to the specific `QProcess` instance it is associated with.
This ensures the handler always operates on the correct process,
fixing the data race and ensuring correct behavior during retries.
A crash could occur during the destruction of the VirtualMonitorPlugin
if a running QProcess finished at a specific moment.
The plugin's destructor calls `stop()`, which in turn calls
`m_process->waitForFinished()`. This is a blocking call that processes
events while waiting. If the process terminates during this wait, it
emits the `finished` signal.
The lambda connected to this signal could, under certain exit
conditions, attempt to call `requestVirtualMonitor()` again. This
re-entrant call on an object that is in the middle of its destruction
sequence leads to undefined behavior and a crash.
This commit fixes the issue by disconnecting the `finished` signal
handler at the beginning of the `stop()` function. This ensures that the
signal handler cannot be invoked during the destruction process,
preventing the re-entrant call.
The `m_process` QProcess object is created with the
`VirtualMonitorPlugin` instance as its parent. Qt's object ownership
model ensures that the parent will automatically delete the child object
during its own destruction.
The `stop()` function was also manually calling `delete m_process`,
which could lead to a double-free and a subsequent crash, particularly
during the plugin's destruction sequence.
This commit replaces the manual `delete` with `deleteLater()`. This
schedules the object's deletion for when control returns to the event
loop, preventing the crash.
The plugin should always be loaded, as:
- we can provide a virtual display over VNC, even if the device isn't capable to use virtual displays itself (`krfb-virtualmonitor`)
- we would do the capabilities check regardless, if !670 gets merged
- hiding the DBus path doesn't trigger the `PluginChecker.qml` to think the plugin is unavailable, it just doesn't work
- -> is this a bug or intended behaviour?
BUG: 485829
## Summary
Currently, the plugin just fails silently if the local device is missing the `krfb` package or if the remote device misses an `vnc://` protocol/scheme handler. You click the button and nothing happens.
One issue is, that the plugin is considered `virtualmonitor.available` in the `DeviceDelegate.qml`, even if the check for `krfb-virtualmonitor` fails and no dbus-path is provided. I investigated the behavior a bit, but ignored it in the end as this MR benefits from being shown for device constellations that _could_ provide this feature.
A warning is shown with brief instructions, how to get the plugin working correctly.
- Check if krfb-virtualmonitor is available locally
- Check default scheme handler for vnc:// on device (Linux)
- Show warnings / reasons, if no connection could be established
## Test Plan
Regarding if the devices have mentioned packages installed, we should see different behaviors.
If the remote device has no VNC client, it can not connect to out server. _A warning should be shown._
If the local device hasn't the `krfb-virtualmonitor` available, the remote device couldn't connect. _A warning should be shown._
If both problems are present, _both warnings should be shown._
If none of these are present, no warning should be shown and we should try to establish a connection.
The connection attempts failed? _A warning should be shown._
BUG: 485830
The current implementation of the plugin is severly broken.
1. The generated URL links to the localhost
2. The port is not set
-----
The URL is now generated on the request receiving side, not send in the request.
This allows finding a valid IP address.
Furthermore, I changed the protocol by splitting it up. This could become useful, if we ever want to support other rdp protocols/platforms.
Note: This is a breaking change, but the current implementation is not working at all.. so it does not actually break something.
Those plugins re really simple and don't need any initialization logic.
With the using statement, we do not need to add a constructor and pass the parent/args to the baseclass
- We do not need the return type. If a plugin declares it can handle the
packet it should do so. We don't have any fallback logic in place and
the packet types are namespaced with the plugin IDs anyway.
- Provide a default implementation with a warning, not all plugins need
to overwrite this
The rationale is explained in https://planet.kde.org/friedrich-kossebau-2023-06-28-include-also-moc-files-of-headers/
In case of KDEConnect, it impressively speeds up compilation. Before it
took 390 seconds on a clean build and with this change it took 330 seconds.
This is due to the mocs_compilation having to include the header files
and thus all their headers. Due to the lots of small plugins we have,
this means that the same headers must be compiled plenty of times.
When we include the moc files directly in the C++ file, they are already
available.
