From 247afb33a2173b3f0c60d1e819ff64cf67258dea Mon Sep 17 00:00:00 2001 From: nikstur Date: Wed, 25 Jan 2023 23:39:46 +0100 Subject: [PATCH] tool: make systemd version parsing robust To make handling systemd versions more robust, they are parsed into a u32 tuple instead of an f32. Additionally, some unit tests for correct parsing and comparing of versions are added. --- rust/tool/src/install.rs | 35 ++------------ rust/tool/src/main.rs | 1 + rust/tool/src/systemd.rs | 98 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 32 deletions(-) create mode 100644 rust/tool/src/systemd.rs diff --git a/rust/tool/src/install.rs b/rust/tool/src/install.rs index 2903b9d..7a0b063 100644 --- a/rust/tool/src/install.rs +++ b/rust/tool/src/install.rs @@ -1,9 +1,7 @@ -use std::ffi::CStr; use std::fs; use std::os::unix::prelude::PermissionsExt; use std::path::{Path, PathBuf}; use std::process::Command; -use std::str::FromStr; use anyhow::{Context, Result}; use nix::unistd::sync; @@ -14,6 +12,7 @@ use crate::generation::{Generation, GenerationLink}; use crate::os_release::OsRelease; use crate::pe; use crate::signature::KeyPair; +use crate::systemd::SystemdVersion; use crate::utils::SecureTempDirExt; pub struct Installer { @@ -330,43 +329,15 @@ fn newer_systemd_boot(from: &Path, to: &Path) -> Result { } // If the version from the source binary cannot be read, something is irrecoverably wrong. - let from_version = systemd_boot_version(from) + let from_version = SystemdVersion::from_systemd_boot_binary(from) .with_context(|| format!("Failed to read systemd-boot version from {from:?}."))?; // If the version cannot be read from the destination binary, it is malformed. It should be // forcibly reinstalled. - let to_version = match systemd_boot_version(to) { + let to_version = match SystemdVersion::from_systemd_boot_binary(to) { Ok(version) => version, _ => return Ok(true), }; Ok(from_version > to_version) } - -/// Read the version of a systemd-boot binary from its `.osrel` section. -/// -/// The version is parsed into a f32 because systemd does not follow strict semver conventions. A -/// f32, however, should parse all systemd versions and enables usefully comparing them. -/// This is a hack and we should replace it with a better solution once we find one. -fn systemd_boot_version(path: &Path) -> Result { - let file_data = fs::read(path).with_context(|| format!("Failed to read file {path:?}"))?; - let section_data = pe::read_section_data(&file_data, ".osrel") - .with_context(|| format!("PE section '.osrel' is empty: {path:?}"))?; - - // The `.osrel` section in the systemd-boot binary is a NUL terminated string and thus needs - // special handling. - let section_data_cstr = - CStr::from_bytes_with_nul(section_data).context("Failed to parse C string.")?; - let section_data_string = section_data_cstr - .to_str() - .context("Failed to convert C string to Rust string.")?; - - let os_release = OsRelease::from_str(section_data_string) - .with_context(|| format!("Failed to parse os-release from {section_data_string}"))?; - - let version_string = os_release - .0 - .get("VERSION") - .context("Failed to extract VERSION key from: {os_release:#?}")?; - Ok(f32::from_str(version_string)?) -} diff --git a/rust/tool/src/main.rs b/rust/tool/src/main.rs index d886bba..bb1d671 100644 --- a/rust/tool/src/main.rs +++ b/rust/tool/src/main.rs @@ -6,6 +6,7 @@ mod install; mod os_release; mod pe; mod signature; +mod systemd; mod utils; use anyhow::Result; diff --git a/rust/tool/src/systemd.rs b/rust/tool/src/systemd.rs new file mode 100644 index 0000000..24a29eb --- /dev/null +++ b/rust/tool/src/systemd.rs @@ -0,0 +1,98 @@ +use std::ffi::CStr; +use std::fs; +use std::path::Path; +use std::str::FromStr; + +use anyhow::{Context, Result}; + +use crate::os_release::OsRelease; +use crate::pe; + +/// A systemd version. +/// +/// The version is parsed into a u32 tuple because systemd does not follow strict semver +/// conventions. A major version without a minor version, e.g. "252" is represented as `(252, 0)`. +#[derive(PartialEq, PartialOrd, Eq, Debug)] +pub struct SystemdVersion(u32, u32); + +impl SystemdVersion { + /// Read the systemd version from the `.osrel` section of a systemd-boot binary. + pub fn from_systemd_boot_binary(path: &Path) -> Result { + let file_data = fs::read(path).with_context(|| format!("Failed to read file {path:?}"))?; + let section_data = pe::read_section_data(&file_data, ".osrel") + .with_context(|| format!("PE section '.osrel' is empty: {path:?}"))?; + + // The `.osrel` section in the systemd-boot binary is a NUL terminated string and thus needs + // special handling. + let section_data_cstr = + CStr::from_bytes_with_nul(section_data).context("Failed to parse C string.")?; + let section_data_string = section_data_cstr + .to_str() + .context("Failed to convert C string to Rust string.")?; + + let os_release = OsRelease::from_str(section_data_string) + .with_context(|| format!("Failed to parse os-release from {section_data_string}"))?; + + let version_str = os_release + .0 + .get("VERSION") + .context("Failed to extract VERSION key from: {os_release:#?}")?; + + Self::from_str(version_str) + } +} + +impl FromStr for SystemdVersion { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + let split_version = s + .split('.') + .take(2) + .map(u32::from_str) + .collect::, std::num::ParseIntError>>() + .context("Failed to parse version string into u32 vector.")?; + + let major = split_version + .first() + .context("Failed to parse major version.")?; + let minor = split_version.get(1).unwrap_or(&0); + + Ok(Self(major.to_owned(), minor.to_owned())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_version_correctly() { + assert_eq!(parse_version("253"), SystemdVersion(253, 0)); + assert_eq!(parse_version("252.4"), SystemdVersion(252, 4)); + assert_eq!(parse_version("251.11"), SystemdVersion(251, 11)); + } + + #[test] + fn compare_version_correctly() { + assert!(parse_version("253") > parse_version("252")); + assert!(parse_version("253") > parse_version("252.4")); + assert!(parse_version("251.8") == parse_version("251.8")); + } + + #[test] + fn fail_to_parse_version() { + parse_version_error(""); + parse_version_error("213;k;13"); + parse_version_error("-1.3.123"); + parse_version_error("253-rc1"); + } + + fn parse_version(input: &str) -> SystemdVersion { + SystemdVersion::from_str(input).unwrap() + } + + fn parse_version_error(input: &str) { + assert!(SystemdVersion::from_str(input).is_err()); + } +}