From 18a85b213f5ae83dc1758da0a401b3f7b06df166 Mon Sep 17 00:00:00 2001 From: keisku Date: Wed, 3 Jul 2024 13:28:03 +0000 Subject: [PATCH 1/8] validate execCPUAffinity Signed-off-by: keisku --- Cargo.toml | 1 + src/runtime/process.rs | 154 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 151 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9b2343e068..a9b0efb9f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ derive_builder = "0.20.0" getset = "0.1.1" strum = "0.26.2" strum_macros = "0.26.2" +regex = "1.10.5" [dev-dependencies] tempfile = "3.2.0" diff --git a/src/runtime/process.rs b/src/runtime/process.rs index 8085f47c49..57baeb6623 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -4,7 +4,9 @@ use crate::{ }; use derive_builder::Builder; use getset::{CopyGetters, Getters, MutGetters, Setters}; -use serde::{Deserialize, Serialize}; +use regex::Regex; +use serde::de; +use serde::{Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; @@ -566,29 +568,75 @@ impl Default for LinuxSchedulerFlag { default, pattern = "owned", setter(into, strip_option), - build_fn(error = "OciSpecError") + build_fn(validate = "Self::validate", error = "OciSpecError") )] #[getset(get = "pub", set = "pub")] /// ExecCPUAffinity specifies CPU affinity used to execute the process. /// This setting is not applicable to the container's init process. pub struct ExecCPUAffinity { - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "deserialize" + )] /// cpu_affinity_initial is a list of CPUs a runtime parent process to be run on /// initially, before the transition to container's cgroup. /// This is a a comma-separated list, with dashes to represent ranges. /// For example, `0-3,7` represents CPUs 0,1,2,3, and 7. cpu_affinity_initial: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "deserialize" + )] /// cpu_affinity_final is a list of CPUs the process will be run on after the transition /// to container's cgroup. The format is the same as for `initial`. If omitted or empty, /// the container's default CPU affinity, as defined by cpu.cpus property, is used. cpu_affinity_final: Option, } +impl ExecCPUAffinityBuilder { + fn validate(&self) -> Result<(), OciSpecError> { + if let Some(Some(ref s)) = self.cpu_affinity_initial { + validate_cpu_affinity(s).map_err(|e| OciSpecError::Other(e.to_string()))?; + } + + if let Some(Some(ref s)) = self.cpu_affinity_final { + validate_cpu_affinity(s).map_err(|e| OciSpecError::Other(e.to_string()))?; + } + + Ok(()) + } +} + +fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let value: Option = Option::deserialize(deserializer)?; + + if let Some(ref s) = value { + validate_cpu_affinity(s).map_err(de::Error::custom)?; + } + + Ok(value) +} + +fn validate_cpu_affinity(s: &str) -> Result<(), String> { + let re = Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").map_err(|e| e.to_string())?; + + if !re.is_match(s) { + return Err(format!("Invalid CPU affinity format: {}", s)); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; + use serde_json::json; // PosixRlimitType test cases #[test] @@ -621,4 +669,102 @@ mod tests { let unknown_rlimit = invalid_posix_rlimit_type_str.parse::(); assert!(unknown_rlimit.is_err()); } + + #[test] + fn exec_cpu_affinity_valid_initial_final() { + let json = json!({"cpu_affinity_initial": "0-3,7", "cpu_affinity_final": "4-6,8"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + + let json = json!({"cpu_affinity_initial": "0-3", "cpu_affinity_final": "4-6"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + + let json = json!({"cpu_affinity_initial": "0", "cpu_affinity_final": "4"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + } + + #[test] + fn exec_cpu_affinity_invalid_initial() { + let json = json!({"cpu_affinity_initial": "0-3,,7", "cpu_affinity_final": "4-6,8"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_err()); + } + + #[test] + fn exec_cpu_affinity_invalid_final() { + let json = json!({"cpu_affinity_initial": "0-3,7", "cpu_affinity_final": "4-6.,8"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_err()); + } + + #[test] + fn exec_cpu_affinity_valid_final() { + let json = json!({"cpu_affinity_final": "0,1,2,3"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + assert!(result.unwrap().cpu_affinity_initial.is_none()); + } + + #[test] + fn exec_cpu_affinity_valid_initial() { + let json = json!({"cpu_affinity_initial": "0-1,2-5"}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + assert!(result.unwrap().cpu_affinity_final.is_none()); + } + + #[test] + fn exec_cpu_affinity_empty() { + let json = json!({}); + let result: Result = serde_json::from_value(json); + assert!(result.is_ok()); + let affinity = result.unwrap(); + assert!(affinity.cpu_affinity_initial.is_none()); + assert!(affinity.cpu_affinity_final.is_none()); + } + + #[test] + fn test_build_valid_input() { + let affinity = ExecCPUAffinityBuilder::default() + .cpu_affinity_initial("0-3,7,8,9,10".to_string()) + .cpu_affinity_final("4-6,8".to_string()) + .build(); + assert!(affinity.is_ok()); + let affinity = affinity.unwrap(); + assert_eq!( + affinity.cpu_affinity_initial, + Some("0-3,7,8,9,10".to_string()) + ); + assert_eq!(affinity.cpu_affinity_final, Some("4-6,8".to_string())); + } + + #[test] + fn test_build_invalid_initial() { + let affinity = ExecCPUAffinityBuilder::default() + .cpu_affinity_initial("0-3,i".to_string()) + .cpu_affinity_final("4-6,8".to_string()) + .build(); + let err = affinity.unwrap_err(); + assert_eq!(err.to_string(), "Invalid CPU affinity format: 0-3,i"); + } + + #[test] + fn test_build_invalid_final() { + let affinity = ExecCPUAffinityBuilder::default() + .cpu_affinity_initial("0-3,7".to_string()) + .cpu_affinity_final("0-l1".to_string()) + .build(); + let err = affinity.unwrap_err(); + assert_eq!(err.to_string(), "Invalid CPU affinity format: 0-l1"); + } + + #[test] + fn test_build_empty() { + let affinity = ExecCPUAffinityBuilder::default().build(); + let affinity = affinity.unwrap(); + assert!(affinity.cpu_affinity_initial.is_none()); + assert!(affinity.cpu_affinity_final.is_none()); + } } From 04ee13dc9ebc2dec34281c9f55151b42a521665c Mon Sep 17 00:00:00 2001 From: keisku Date: Wed, 3 Jul 2024 14:21:17 +0000 Subject: [PATCH 2/8] format import Signed-off-by: keisku --- src/runtime/process.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/runtime/process.rs b/src/runtime/process.rs index 57baeb6623..e4cf88c6d8 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -5,8 +5,7 @@ use crate::{ use derive_builder::Builder; use getset::{CopyGetters, Getters, MutGetters, Setters}; use regex::Regex; -use serde::de; -use serde::{Deserialize, Deserializer, Serialize}; +use serde::{de, Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; From e01daa1ec34990469854e4e3d7d4051cbe9dce55 Mon Sep 17 00:00:00 2001 From: keisku Date: Wed, 3 Jul 2024 14:26:27 +0000 Subject: [PATCH 3/8] use once_cell to compile regex only once Signed-off-by: keisku --- Cargo.toml | 1 + src/runtime/process.rs | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a9b0efb9f7..dc16cd7f9c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ getset = "0.1.1" strum = "0.26.2" strum_macros = "0.26.2" regex = "1.10.5" +once_cell = "1.19.0" [dev-dependencies] tempfile = "3.2.0" diff --git a/src/runtime/process.rs b/src/runtime/process.rs index e4cf88c6d8..8c1a1f78ae 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -8,6 +8,7 @@ use regex::Regex; use serde::{de, Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; +use once_cell::sync::Lazy; #[derive( Builder, @@ -622,10 +623,12 @@ where Ok(value) } -fn validate_cpu_affinity(s: &str) -> Result<(), String> { - let re = Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").map_err(|e| e.to_string())?; +static CPU_AFFINITY_REGEX: Lazy = Lazy::new(|| { + Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").expect("Failed to create regex for execCPUAffinity") +}); - if !re.is_match(s) { +fn validate_cpu_affinity(s: &str) -> Result<(), String> { + if !CPU_AFFINITY_REGEX.is_match(s) { return Err(format!("Invalid CPU affinity format: {}", s)); } From a24746883e6c392f3876635190705e9b3647a18b Mon Sep 17 00:00:00 2001 From: keisku Date: Wed, 3 Jul 2024 14:28:55 +0000 Subject: [PATCH 4/8] use same word Signed-off-by: keisku --- src/runtime/process.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/runtime/process.rs b/src/runtime/process.rs index 8c1a1f78ae..ade1f95e2c 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -623,13 +623,13 @@ where Ok(value) } -static CPU_AFFINITY_REGEX: Lazy = Lazy::new(|| { +static EXEC_CPU_AFFINITY_REGEX: Lazy = Lazy::new(|| { Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").expect("Failed to create regex for execCPUAffinity") }); fn validate_cpu_affinity(s: &str) -> Result<(), String> { - if !CPU_AFFINITY_REGEX.is_match(s) { - return Err(format!("Invalid CPU affinity format: {}", s)); + if !EXEC_CPU_AFFINITY_REGEX.is_match(s) { + return Err(format!("Invalid execCPUAffinity format: {}", s)); } Ok(()) @@ -749,7 +749,7 @@ mod tests { .cpu_affinity_final("4-6,8".to_string()) .build(); let err = affinity.unwrap_err(); - assert_eq!(err.to_string(), "Invalid CPU affinity format: 0-3,i"); + assert_eq!(err.to_string(), "Invalid execCPUAffinity format: 0-3,i"); } #[test] @@ -759,7 +759,7 @@ mod tests { .cpu_affinity_final("0-l1".to_string()) .build(); let err = affinity.unwrap_err(); - assert_eq!(err.to_string(), "Invalid CPU affinity format: 0-l1"); + assert_eq!(err.to_string(), "Invalid execCPUAffinity format: 0-l1"); } #[test] From 5a8a9421d4a98ce69c29f31fa7f8b7e6fca693b4 Mon Sep 17 00:00:00 2001 From: keisku Date: Wed, 3 Jul 2024 22:17:32 +0000 Subject: [PATCH 5/8] cargo fmt Signed-off-by: keisku --- src/runtime/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/process.rs b/src/runtime/process.rs index ade1f95e2c..1cdc943d05 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -4,11 +4,11 @@ use crate::{ }; use derive_builder::Builder; use getset::{CopyGetters, Getters, MutGetters, Setters}; +use once_cell::sync::Lazy; use regex::Regex; use serde::{de, Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; -use once_cell::sync::Lazy; #[derive( Builder, From 73c974022d784d562d8400af0c8bd844fb1d7cf2 Mon Sep 17 00:00:00 2001 From: keisku Date: Thu, 4 Jul 2024 12:42:59 +0000 Subject: [PATCH 6/8] avoid using regex Signed-off-by: keisku --- Cargo.toml | 2 -- src/runtime/process.rs | 32 ++++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index dc16cd7f9c..9b2343e068 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,8 +32,6 @@ derive_builder = "0.20.0" getset = "0.1.1" strum = "0.26.2" strum_macros = "0.26.2" -regex = "1.10.5" -once_cell = "1.19.0" [dev-dependencies] tempfile = "3.2.0" diff --git a/src/runtime/process.rs b/src/runtime/process.rs index 1cdc943d05..ed3721187b 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -4,8 +4,6 @@ use crate::{ }; use derive_builder::Builder; use getset::{CopyGetters, Getters, MutGetters, Setters}; -use once_cell::sync::Lazy; -use regex::Regex; use serde::{de, Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; @@ -623,13 +621,31 @@ where Ok(value) } -static EXEC_CPU_AFFINITY_REGEX: Lazy = Lazy::new(|| { - Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").expect("Failed to create regex for execCPUAffinity") -}); - fn validate_cpu_affinity(s: &str) -> Result<(), String> { - if !EXEC_CPU_AFFINITY_REGEX.is_match(s) { - return Err(format!("Invalid execCPUAffinity format: {}", s)); + let iter = s.split(','); + + for part in iter { + let mut range_iter = part.split('-'); + let start = range_iter + .next() + .ok_or_else(|| format!("Invalid execCPUAffinity format: {}", s))?; + + // Check if the start is a valid number + if start.parse::().is_err() { + return Err(format!("Invalid execCPUAffinity format: {}", s)); + } + + // Check if there's a range and if the end is a valid number + if let Some(end) = range_iter.next() { + if end.parse::().is_err() { + return Err(format!("Invalid execCPUAffinity format: {}", s)); + } + } + + // Ensure there's no extra data after the end of a range + if range_iter.next().is_some() { + return Err(format!("Invalid execCPUAffinity format: {}", s)); + } } Ok(()) From 646976d003e5c3b4a859ef00007c46c18519959f Mon Sep 17 00:00:00 2001 From: keisku Date: Thu, 4 Jul 2024 12:44:49 +0000 Subject: [PATCH 7/8] add unit tests Signed-off-by: keisku --- src/runtime/process.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/runtime/process.rs b/src/runtime/process.rs index ed3721187b..108a7decab 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -766,6 +766,13 @@ mod tests { .build(); let err = affinity.unwrap_err(); assert_eq!(err.to_string(), "Invalid execCPUAffinity format: 0-3,i"); + + let affinity = ExecCPUAffinityBuilder::default() + .cpu_affinity_initial("-".to_string()) + .cpu_affinity_final("4-6,8".to_string()) + .build(); + let err = affinity.unwrap_err(); + assert_eq!(err.to_string(), "Invalid execCPUAffinity format: -"); } #[test] @@ -776,6 +783,13 @@ mod tests { .build(); let err = affinity.unwrap_err(); assert_eq!(err.to_string(), "Invalid execCPUAffinity format: 0-l1"); + + let affinity = ExecCPUAffinityBuilder::default() + .cpu_affinity_initial("0-3,7".to_string()) + .cpu_affinity_final(",1,2".to_string()) + .build(); + let err = affinity.unwrap_err(); + assert_eq!(err.to_string(), "Invalid execCPUAffinity format: ,1,2"); } #[test] From d066e9c9d2254d4eb2f8268ebf2f8ddfb08cf99b Mon Sep 17 00:00:00 2001 From: keisku Date: Thu, 4 Jul 2024 13:20:58 +0000 Subject: [PATCH 8/8] Revert "avoid using regex" This reverts commit 73c974022d784d562d8400af0c8bd844fb1d7cf2. Signed-off-by: keisku --- Cargo.toml | 2 ++ src/runtime/process.rs | 32 ++++++++------------------------ 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9b2343e068..dc16cd7f9c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,8 @@ derive_builder = "0.20.0" getset = "0.1.1" strum = "0.26.2" strum_macros = "0.26.2" +regex = "1.10.5" +once_cell = "1.19.0" [dev-dependencies] tempfile = "3.2.0" diff --git a/src/runtime/process.rs b/src/runtime/process.rs index 108a7decab..20d197a1c1 100644 --- a/src/runtime/process.rs +++ b/src/runtime/process.rs @@ -4,6 +4,8 @@ use crate::{ }; use derive_builder::Builder; use getset::{CopyGetters, Getters, MutGetters, Setters}; +use once_cell::sync::Lazy; +use regex::Regex; use serde::{de, Deserialize, Deserializer, Serialize}; use std::path::PathBuf; use strum_macros::{Display as StrumDisplay, EnumString}; @@ -621,31 +623,13 @@ where Ok(value) } -fn validate_cpu_affinity(s: &str) -> Result<(), String> { - let iter = s.split(','); - - for part in iter { - let mut range_iter = part.split('-'); - let start = range_iter - .next() - .ok_or_else(|| format!("Invalid execCPUAffinity format: {}", s))?; - - // Check if the start is a valid number - if start.parse::().is_err() { - return Err(format!("Invalid execCPUAffinity format: {}", s)); - } +static EXEC_CPU_AFFINITY_REGEX: Lazy = Lazy::new(|| { + Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").expect("Failed to create regex for execCPUAffinity") +}); - // Check if there's a range and if the end is a valid number - if let Some(end) = range_iter.next() { - if end.parse::().is_err() { - return Err(format!("Invalid execCPUAffinity format: {}", s)); - } - } - - // Ensure there's no extra data after the end of a range - if range_iter.next().is_some() { - return Err(format!("Invalid execCPUAffinity format: {}", s)); - } +fn validate_cpu_affinity(s: &str) -> Result<(), String> { + if !EXEC_CPU_AFFINITY_REGEX.is_match(s) { + return Err(format!("Invalid execCPUAffinity format: {}", s)); } Ok(())