From a9a832f392447101762f6a4f6937603a06ed8156 Mon Sep 17 00:00:00 2001 From: Sebastian Eydam Date: Mon, 30 Mar 2026 15:14:43 +0200 Subject: [PATCH] vmm: validate VmSendMigrationData Validates that there are no conflicting options set, and that the destination URL is valid. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam --- cloud-hypervisor/src/bin/ch-remote.rs | 2 +- vmm/src/api/mod.rs | 116 ++++++++++++++++++++------ vmm/src/lib.rs | 7 +- 3 files changed, 96 insertions(+), 29 deletions(-) diff --git a/cloud-hypervisor/src/bin/ch-remote.rs b/cloud-hypervisor/src/bin/ch-remote.rs index afc41e7e9..236e7438e 100644 --- a/cloud-hypervisor/src/bin/ch-remote.rs +++ b/cloud-hypervisor/src/bin/ch-remote.rs @@ -72,7 +72,7 @@ enum Error { #[error("Invalid disk size")] InvalidDiskSize(#[source] ByteSizedParseError), #[error("Error parsing send migration configuration")] - SendMigrationConfig(#[from] vmm::api::VmSendMigrationParseError), + SendMigrationConfig(#[from] vmm::api::VmSendMigrationConfigError), } enum TargetApi<'a> { diff --git a/vmm/src/api/mod.rs b/vmm/src/api/mod.rs index 10457d7d3..f66fbe9ab 100644 --- a/vmm/src/api/mod.rs +++ b/vmm/src/api/mod.rs @@ -295,8 +295,13 @@ impl FromStr for TimeoutStrategy { } #[derive(Debug, Error)] -#[error("Error parsing send migration parameters")] -pub struct VmSendMigrationParseError(#[source] OptionParserError); +pub enum VmSendMigrationConfigError { + #[error("Error parsing send migration parameters")] + ParseError(#[source] OptionParserError), + + #[error("Error validating send migration parameters")] + ValidationError(String), +} /// Configuration for an outgoing migration. #[derive(Clone, Deserialize, Serialize, Debug)] @@ -348,7 +353,7 @@ impl VmSendMigrationData { NonZeroU32::new(1).unwrap() } - pub fn parse(migration: &str) -> Result { + pub fn parse(migration: &str) -> Result { let mut parser = OptionParser::new(); parser .add("destination_url") @@ -357,24 +362,26 @@ impl VmSendMigrationData { .add("timeout_s") .add("timeout_strategy") .add("connections"); - parser.parse(migration).map_err(VmSendMigrationParseError)?; + parser + .parse(migration) + .map_err(VmSendMigrationConfigError::ParseError)?; let destination_url = parser.get("destination_url").ok_or_else(|| { - VmSendMigrationParseError(OptionParserError::InvalidSyntax( + VmSendMigrationConfigError::ParseError(OptionParserError::InvalidSyntax( "destination_url is required".to_string(), )) })?; let local = parser .convert::("local") - .map_err(VmSendMigrationParseError)? + .map_err(VmSendMigrationConfigError::ParseError)? .unwrap_or(Toggle(false)) .0; let downtime_ms = match parser .convert::("downtime_ms") - .map_err(VmSendMigrationParseError)? + .map_err(VmSendMigrationConfigError::ParseError)? { Some(v) => NonZeroU64::new(v).ok_or_else(|| { - VmSendMigrationParseError(OptionParserError::InvalidValue( + VmSendMigrationConfigError::ParseError(OptionParserError::InvalidValue( "downtime_ms must be non-zero".to_string(), )) })?, @@ -382,10 +389,10 @@ impl VmSendMigrationData { }; let timeout_s = match parser .convert::("timeout_s") - .map_err(VmSendMigrationParseError)? + .map_err(VmSendMigrationConfigError::ParseError)? { Some(v) => NonZeroU64::new(v).ok_or_else(|| { - VmSendMigrationParseError(OptionParserError::InvalidValue( + VmSendMigrationConfigError::ParseError(OptionParserError::InvalidValue( "timeout_s must be non-zero".to_string(), )) })?, @@ -393,28 +400,32 @@ impl VmSendMigrationData { }; let timeout_strategy = parser .convert("timeout_strategy") - .map_err(VmSendMigrationParseError)? + .map_err(VmSendMigrationConfigError::ParseError)? .unwrap_or_default(); let connections = match parser .convert::("connections") - .map_err(VmSendMigrationParseError)? + .map_err(VmSendMigrationConfigError::ParseError)? { Some(v) => NonZeroU32::new(v).ok_or_else(|| { - VmSendMigrationParseError(OptionParserError::InvalidValue( + VmSendMigrationConfigError::ParseError(OptionParserError::InvalidValue( "connections must be non-zero".to_string(), )) })?, None => Self::default_connections(), }; - Ok(Self { + let data = Self { destination_url, local, downtime_ms, timeout_s, timeout_strategy, connections, - }) + }; + + data.validate()?; + + Ok(data) } pub fn downtime(&self) -> Duration { @@ -424,6 +435,47 @@ impl VmSendMigrationData { pub fn timeout(&self) -> Duration { Duration::from_secs(self.timeout_s.get()) } + + pub fn validate(&self) -> Result<(), VmSendMigrationConfigError> { + match self.destination_url.as_str() { + url if url + .strip_prefix("tcp:") + .is_some_and(|addr| !addr.is_empty()) => {} + url if url + .strip_prefix("unix:") + .is_some_and(|path| !path.is_empty()) => + { + if self.connections.get() > 1 { + return Err(VmSendMigrationConfigError::ValidationError( + "UNIX sockets and connections option cannot be used at the same time." + .to_string(), + )); + } + } + _ => { + return Err(VmSendMigrationConfigError::ValidationError( + "destination_url must use tcp:: or unix:.".to_string(), + )); + } + } + + if self.local { + if !self.destination_url.starts_with("unix:") { + return Err(VmSendMigrationConfigError::ValidationError( + "local option is only supported with UNIX sockets.".to_string(), + )); + } + + if self.connections.get() > 1 { + return Err(VmSendMigrationConfigError::ValidationError( + "local option and connections option cannot be used at the same time." + .to_string(), + )); + } + } + + Ok(()) + } } pub enum ApiResponsePayload { @@ -1701,19 +1753,19 @@ mod unit_tests { fn test_vm_send_migration_data_parse() { // Fully specified let data = VmSendMigrationData::parse( - "destination_url=tcp://192.168.1.1:8080,local=on,downtime_ms=200,timeout_s=3600,timeout_strategy=cancel,connections=2" + "destination_url=unix:/tmp/migrate.sock,local=on,downtime_ms=200,timeout_s=3600,timeout_strategy=cancel" ).expect("valid migration string should parse"); - assert_eq!(data.destination_url, "tcp://192.168.1.1:8080"); + assert_eq!(data.destination_url, "unix:/tmp/migrate.sock"); assert!(data.local); assert_eq!(data.downtime_ms.get(), 200); assert_eq!(data.timeout_s.get(), 3600); assert_eq!(data.timeout_strategy, TimeoutStrategy::Cancel); - assert_eq!(data.connections.get(), 2); + assert_eq!(data.connections.get(), 1); // Defaults applied when optional fields are omitted - let data = VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080") + let data = VmSendMigrationData::parse("destination_url=tcp:192.168.1.1:8080") .expect("minimal migration string should parse"); - assert_eq!(data.destination_url, "tcp://192.168.1.1:8080"); + assert_eq!(data.destination_url, "tcp:192.168.1.1:8080"); assert!(!data.local); assert_eq!(data.downtime_ms, VmSendMigrationData::default_downtime_ms()); assert_eq!(data.timeout_s, VmSendMigrationData::default_timeout_s()); @@ -1725,7 +1777,7 @@ mod unit_tests { // Zero downtime_ms is rejected let _data = - VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080,downtime_ms=0") + VmSendMigrationData::parse("destination_url=tcp:192.168.1.1:8080,downtime_ms=0") .expect_err("zero downtime_ms should be rejected"); // Zero timeout_s is rejected @@ -1744,18 +1796,28 @@ mod unit_tests { // Timeout strategy let _data = VmSendMigrationData::parse( - "destination_url=tcp://192.168.1.1:8080,timeout_strategy=invalid", + "destination_url=tcp:192.168.1.1:8080,timeout_strategy=invalid", ) - .expect_err("zero downtime_ms should be rejected"); + .expect_err("invalid timeout strategy should be rejected"); + + // Invalid destination URL scheme is rejected + VmSendMigrationData::parse("destination_url=file:///tmp/migration").unwrap_err(); + + // Local migration requires a UNIX socket destination + VmSendMigrationData::parse("destination_url=tcp:192.168.1.1:8080,local=yes").unwrap_err(); + + // Local migration cannot use multiple connections + VmSendMigrationData::parse("destination_url=unix:/tmp/sock,local=yes,connections=2") + .unwrap_err(); // Happy path with some defaults let data = - VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080,downtime_ms=150") + VmSendMigrationData::parse("destination_url=tcp:192.168.1.1:8080,downtime_ms=150") .unwrap(); assert_eq!( data, VmSendMigrationData { - destination_url: "tcp://192.168.1.1:8080".to_string(), + destination_url: "tcp:192.168.1.1:8080".to_string(), local: false, downtime_ms: NonZeroU64::new(150).unwrap(), timeout_s: VmSendMigrationData::default_timeout_s(), @@ -1766,12 +1828,12 @@ mod unit_tests { // Happy path, fully specified let data = - VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080,downtime_ms=150,timeout_s=900,timeout_strategy=ignore,connections=4") + VmSendMigrationData::parse("destination_url=tcp:192.168.1.1:8080,downtime_ms=150,timeout_s=900,timeout_strategy=ignore,connections=4") .unwrap(); assert_eq!( data, VmSendMigrationData { - destination_url: "tcp://192.168.1.1:8080".to_string(), + destination_url: "tcp:192.168.1.1:8080".to_string(), local: false, downtime_ms: NonZeroU64::new(150).unwrap(), timeout_s: NonZeroU64::new(900).unwrap(), diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index f1226090c..ce5664249 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -17,7 +17,7 @@ use std::time::Duration; use std::time::Instant; use std::{io, result, thread}; -use anyhow::anyhow; +use anyhow::{Context, anyhow}; #[cfg(feature = "dbus_api")] use api::dbus::{DBusApiOptions, DBusApiShutdownChannels}; use api::http::HttpApiHandle; @@ -2387,6 +2387,11 @@ impl RequestHandler for Vmm { &mut self, send_data_migration: VmSendMigrationData, ) -> result::Result<(), MigratableError> { + send_data_migration + .validate() + .context("Invalid send migration configuration") + .map_err(MigratableError::MigrateSend)?; + info!( "Sending migration: destination_url={},local={},downtime={}ms,timeout={}s,timeout_strategy={:?}", send_data_migration.destination_url,