TheLanc3/rustdesk
1
0
Fork 0
mirror of https://github.com/rustdesk/rustdesk.git synced 2026-09-08 05:20:59 +03:00
Code Issues Packages Projects Releases Wiki Activity

drm: make the outer handshake budget dominate the inner one

Browse Source 
Two findings from the review bot on our own fork, both worth taking.

The caller waited HANDSHAKE_TIMEOUT_MS + 500 for the receive thread to hand back
the display list, but that thread is allowed to spend more than that: the connect
budget, and then recv_msg_timeout2 applies its argument twice in the worst case,
once waiting for the first byte and once for the body. So on a slow connect the
outer timer fired first and abandoned a handshake that was still inside its own
budget. The wait is now derived from those parts rather than written as a
constant, so changing either one cannot silently invert the relationship again,
and the two connect sites use the named constant instead of a literal.

The cursor cache insert shadowed hcursor under a cfg, so the same line meant the
requested id in one build and the served id in the other. It is a separate name
now, with the reason on it.

Not taken, and why: the bot also suggested making DrmCursorData carry width and
height as u32 to match the wire. They are i32 because that is what they feed,
protobuf CursorData declares both as int32 and platform/linux.rs assigns them
straight across. One cast has to exist somewhere, and it belongs at the boundary
where the values are already being validated, not at the consumer.

101 tests pass, both configs build.
This commit is contained in:
Mariano Abad
2026-07-28 10:21:51 -03:00
parent 98342d4aec
commit a0bb909f4e
2 changed files with 26 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/server/drm_capturer.rs
View File

@@ -33,9 +33,17 @@ use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{Arc, Condvar, Mutex};
use std::time::{Duration, Instant};
// Upper bound on how long `new()` waits for the service to answer with the display list before
// giving up and letting the caller fall back.
// Upper bound on how long the receive thread waits for the service to answer with the display list.
const HANDSHAKE_TIMEOUT_MS: u64 = 3000;
// How long that thread may spend connecting to `_drm` before the handshake starts.
const DRM_CONNECT_TIMEOUT_MS: u64 = 1000;
/// How long a caller waits for the receive thread to hand back the display list. It must DOMINATE
/// what that thread is allowed to spend, or the outer timer fires first and abandons a handshake
/// that was still inside its own budget: the thread spends up to the connect timeout, then
/// `recv_msg_timeout2` applies HANDSHAKE_TIMEOUT_MS TWICE in the worst case (once waiting for the
/// first byte, once for the body). Derived from those parts rather than written as a constant, so a
/// change to either one cannot silently invert the relationship again.
const HANDSHAKE_WAIT_MS: u64 = DRM_CONNECT_TIMEOUT_MS + HANDSHAKE_TIMEOUT_MS * 2 + 500;
struct FrameSlot {
// (width, height, pixel format, packed pixels) of the newest frame not yet consumed by
@@ -271,7 +279,7 @@ impl IpcDrmCapturer {
let stop = stop.clone();
std::thread::spawn(move || recv_thread(display, shared, stop, tx));
}
let displays = match rx.recv_timeout(Duration::from_millis(HANDSHAKE_TIMEOUT_MS + 500)) {
let displays = match rx.recv_timeout(Duration::from_millis(HANDSHAKE_WAIT_MS)) {
Ok(res) => res?,
Err(_) => {
// The recv thread still has its own connect/handshake budget. If we just returned,
@@ -429,7 +437,7 @@ async fn recv_thread(
// remove_drm_cursor); a rebuilt stream for the same display index gets a newer epoch.
let cursor_epoch = next_cursor_epoch();
// Handshake: connect, receive the display list, request the display.
let mut conn = match connect_drm(1000).await {
let mut conn = match connect_drm(DRM_CONNECT_TIMEOUT_MS).await {
Ok(c) => c,
Err(err) => {
let _ = tx.send(Err(err));
@@ -908,13 +916,13 @@ fn query_displays() -> ResultType<Vec<DrmDisplayInfo>> {
std::thread::spawn(move || {
let _ = tx.send(query_displays_async());
});
rx.recv_timeout(Duration::from_millis(HANDSHAKE_TIMEOUT_MS + 1000))
rx.recv_timeout(Duration::from_millis(HANDSHAKE_WAIT_MS))
.map_err(|_| anyhow!("drm display query timed out"))?
}
#[tokio::main(flavor = "current_thread")]
async fn query_displays_async() -> ResultType<Vec<DrmDisplayInfo>> {
let mut conn = connect_drm(1000).await?;
let mut conn = connect_drm(DRM_CONNECT_TIMEOUT_MS).await?;
match conn.recv_msg_timeout2(HANDSHAKE_TIMEOUT_MS).await {
Some(Ok((Data::DrmDisplayList(v), _fd))) => Ok(v),
Some(Ok((other, _fd))) => Err(anyhow!("expected DrmDisplayList, got {:?}", other)),

16
src/server/input_service.rs
View File

@@ -408,18 +408,26 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()>
msg = cached.clone();
} else {
let mut data = crate::get_cursor_data(hcursor)?;
// File the shape under the id ACTUALLY served, not the one requested. Deliberately a
// NEW name rather than shadowing `hcursor`: the insert below reads as the requested
// id everywhere else in this function, and a cfg-gated shadow would make the two
// builds disagree about what that line means.
#[cfg(all(target_os = "linux", feature = "drm"))]
let hcursor = data.id;
let served_id = data.id;
#[cfg(all(target_os = "linux", feature = "drm"))]
{
drm_served_id = hcursor;
drm_served_id = served_id;
}
#[cfg(all(target_os = "linux", feature = "drm"))]
let cache_key = served_id;
#[cfg(not(all(target_os = "linux", feature = "drm")))]
let cache_key = hcursor;
data.colors = hbb_common::compress::compress(&data.colors[..]).into();
let mut tmp = Message::new();
tmp.set_cursor_data(data);
msg = Arc::new(tmp);
state.cached_cursor_data.insert(hcursor, msg.clone());
super::log::trace!("Cursor data updated, hcursor: {}", hcursor);
state.cached_cursor_data.insert(cache_key, msg.clone());
super::log::trace!("Cursor data updated, hcursor: {}", cache_key);
}
#[cfg(not(all(target_os = "linux", feature = "drm")))]
{