Merge pull request #14 from edera-dev/azenla/fixes

Repair automated bug analysis issues.

src/actions.rs

@@ -1,5 +1,5 @@
 use crate::context::SproutContext;
-use anyhow::{Result, bail};
+use anyhow::{Context, Result, bail};
 use serde::{Deserialize, Serialize};
 use std::rc::Rc;
 
@@ -50,7 +50,10 @@ pub fn execute(context: Rc<SproutContext>, name: impl AsRef<str>) -> Result<()>
         bail!("unknown action '{}'", name.as_ref());
     };
     // Finalize the context and freeze it.
-    let context = context.finalize().freeze();
+    let context = context
+        .finalize()
+        .context("unable to finalize context")?
+        .freeze();
 
     // Execute the action.
     if let Some(chainload) = &action.chainload {
@@ -61,6 +64,7 @@ pub fn execute(context: Rc<SproutContext>, name: impl AsRef<str>) -> Result<()>
         return Ok(());
     } else if let Some(edera) = &action.edera {
         edera::edera(context.clone(), edera)?;
+        return Ok(());
     }
 
     #[cfg(feature = "splash")]

src/actions/splash.rs

@@ -52,6 +52,11 @@ fn fit_to_frame(image: &DynamicImage, frame: Rect) -> Rect {
         height: image.height(),
     };
 
+    // Handle the case where the image is zero-sized.
+    if input.height == 0 || input.width == 0 {
+        return input;
+    }
+
     // Calculate the ratio of the image dimensions.
     let input_ratio = input.width as f32 / input.height as f32;
 
@@ -66,6 +71,11 @@ fn fit_to_frame(image: &DynamicImage, frame: Rect) -> Rect {
         height: frame.height,
     };
 
+    // Handle the case where the output is zero-sized.
+    if output.height == 0 || output.width == 0 {
+        return output;
+    }
+
     if input_ratio < frame_ratio {
         output.width = (frame.height as f32 * input_ratio).floor() as u32;
         output.height = frame.height;
@@ -110,7 +120,8 @@ fn draw(image: DynamicImage) -> Result<()> {
     let image = resize_to_fit(&image, fit);
 
     // Create a framebuffer to draw the image on.
-    let mut framebuffer = Framebuffer::new(width, height);
+    let mut framebuffer =
+        Framebuffer::new(width, height).context("unable to create framebuffer")?;
 
     // Iterate over the pixels in the image and put them on the framebuffer.
     for (x, y, pixel) in image.enumerate_pixels() {

src/context.rs

@@ -1,11 +1,15 @@
 use crate::actions::ActionDeclaration;
 use crate::options::SproutOptions;
-use anyhow::Result;
 use anyhow::anyhow;
+use anyhow::{Result, bail};
+use std::cmp::Reverse;
 use std::collections::{BTreeMap, BTreeSet};
 use std::rc::Rc;
 use uefi::proto::device_path::DevicePath;
 
+/// The maximum number of iterations that can be performed in [SproutContext::finalize].
+const CONTEXT_FINALIZE_ITERATION_LIMIT: usize = 100;
+
 /// Declares a root context for Sprout.
 /// This contains data that needs to be shared across Sprout.
 #[derive(Default)]
@@ -151,11 +155,20 @@ impl SproutContext {
     /// Finalizes a context by producing a context with no parent that contains all the values
     /// of all parent contexts merged. This makes it possible to ensure [SproutContext] has no
     /// inheritance with other [SproutContext]s. It will still contain a [RootContext] however.
-    pub fn finalize(&self) -> SproutContext {
+    pub fn finalize(&self) -> Result<SproutContext> {
         // Collect all the values from the context and its parents.
         let mut current_values = self.all_values();
 
+        // To ensure that there is no possible infinite loop, we need to check
+        // the number of iterations. If it exceeds 100, we bail.
+        let mut iterations: usize = 0;
         loop {
+            iterations += 1;
+
+            if iterations > CONTEXT_FINALIZE_ITERATION_LIMIT {
+                bail!("infinite loop detected in context finalization");
+            }
+
             let mut did_change = false;
             let mut values = BTreeMap::new();
             for (key, value) in &current_values {
@@ -176,11 +189,11 @@ impl SproutContext {
         }
 
         // Produce the final context.
-        Self {
+        Ok(Self {
             root: self.root.clone(),
             parent: None,
             values: current_values,
-        }
+        })
     }
 
     /// Stamps the `text` value with the specified `values` map. The returned value indicates
@@ -188,7 +201,25 @@ impl SproutContext {
     fn stamp_values(values: &BTreeMap<String, String>, text: impl AsRef<str>) -> (bool, String) {
         let mut result = text.as_ref().to_string();
         let mut did_change = false;
-        for (key, value) in values {
+
+        // Sort the keys by length. This is to ensure that we stamp the longest keys first.
+        // If we did not do this, "$abc" could be stamped by "$a" into an invalid result.
+        let mut keys = values.keys().collect::<Vec<_>>();
+
+        // Sort by key length, reversed. This results in the longest keys appearing first.
+        keys.sort_by_key(|key| Reverse(key.len()));
+
+        for key in keys {
+            // Empty keys are not supported.
+            if key.is_empty() {
+                continue;
+            }
+
+            // We can fetch the value from the map. It is verifiable that the key exists.
+            let Some(value) = values.get(key) else {
+                unreachable!("keys iterated over is collected on a map that cannot be modified");
+            };
+
             let next_result = result.replace(&format!("${key}"), value);
             if result != next_result {
                 did_change = true;

src/extractors/filesystem_device_match.rs

@@ -81,7 +81,7 @@ pub fn extract(
                     } else {
                         // We should still handle other errors gracefully.
                         Err(error).context("unable to open filesystem partition info")?;
-                        None
+                        unreachable!()
                     }
                 }
             }

src/generators/bls.rs

@@ -6,7 +6,6 @@ use anyhow::{Context, Result};
 use serde::{Deserialize, Serialize};
 use std::rc::Rc;
 use std::str::FromStr;
-use uefi::CString16;
 use uefi::fs::{FileSystem, Path};
 use uefi::proto::device_path::text::{AllowShortcuts, DisplayOnly};
 use uefi::proto::media::fs::SimpleFileSystem;
@@ -89,10 +88,9 @@ pub fn generate(context: Rc<SproutContext>, bls: &BlsConfiguration) -> Result<Ve
             continue;
         }
 
-        // Produce the full path to the entry file.
+        // Create a mutable path so we can append the file name to produce the full path.
-        let full_entry_path = CString16::try_from(format!("{}\\{}", sub_text_path, name).as_str())
+        let mut full_entry_path = entries_path.to_path_buf();
-            .context("unable to construct full entry path")?;
+        full_entry_path.push(entry.file_name());
-        let full_entry_path = Path::new(&full_entry_path);
 
         // Read the entry file.
         let content = fs

src/generators/bls/entry.rs

@@ -36,8 +36,13 @@ impl FromStr for BlsEntry {
             // Trim the line.
             let line = line.trim();
 
-            // Split the line once by a space.
+            // Skip over empty lines and comments.
-            let Some((key, value)) = line.split_once(" ") else {
+            if line.is_empty() || line.starts_with('#') {
+                continue;
+            }
+
+            // Split the line once by whitespace.
+            let Some((key, value)) = line.split_once(char::is_whitespace) else {
                 continue;
             };
 
@@ -99,7 +104,7 @@ impl BlsEntry {
         self.linux
             .clone()
             .or(self.efi.clone())
-            .map(|path| path.replace("/", "\\").trim_start_matches("\\").to_string())
+            .map(|path| path.replace('/', "\\").trim_start_matches('\\').to_string())
     }
 
     /// Fetches the path to an initrd to pass to the kernel, if any.
@@ -107,7 +112,7 @@ impl BlsEntry {
     pub fn initrd_path(&self) -> Option<String> {
         self.initrd
             .clone()
-            .map(|path| path.replace("/", "\\").trim_start_matches("\\").to_string())
+            .map(|path| path.replace('/', "\\").trim_start_matches('\\').to_string())
     }
 
     /// Fetches the options to pass to the kernel, if any.

src/main.rs

@@ -107,7 +107,7 @@ fn main() -> Result<()> {
     context.insert(&extracted);
     let context = context.freeze();
 
-    // Execute the late phase.
+    // Execute the startup phase.
     phase(context.clone(), &config.phases.startup).context("unable to execute startup phase")?;
 
     let mut entries = Vec::new();
@@ -143,7 +143,10 @@ fn main() -> Result<()> {
         // Insert the values from the entry configuration into the
         // sprout context to use with the entry itself.
         context.insert(&entry.declaration().values);
-        let context = context.finalize().freeze();
+        let context = context
+            .finalize()
+            .context("unable to finalize context")?
+            .freeze();
         // Provide the new context to the bootable entry.
         entry.swap_context(context);
         // Restamp the title with any values.

src/options/parser.rs

@@ -72,11 +72,7 @@ pub trait OptionsRepresentable {
             let mut value = None;
 
             // Check if the option is of the form --abc=123
-            if option.contains("=") {
+            if let Some((part_key, part_value)) = option.split_once('=') {
-                let Some((part_key, part_value)) = option.split_once("=") else {
-                    bail!("invalid option: {option}");
-                };
-
                 let part_key = part_key.to_string();
                 let part_value = part_value.to_string();
                 option = part_key;
@@ -131,7 +127,7 @@ pub trait OptionsRepresentable {
                     );
                 }
                 // Exit because the help has been displayed.
-                std::process::exit(1);
+                std::process::exit(0);
             }
 
             // Insert the option and the value into the map.

src/utils.rs

@@ -27,6 +27,12 @@ pub fn text_to_device_path(path: &str) -> Result<PoolDevicePath> {
         .context("unable to convert text to device path")
 }
 
+/// Checks if a [CString16] contains a char `c`.
+/// We need to call to_string() because CString16 doesn't support `contains` with a char.
+fn cstring16_contains_char(string: &CString16, c: char) -> bool {
+    string.to_string().contains(c)
+}
+
 /// Grabs the root part of the `path`.
 /// For example, given "PciRoot(0x0)/Pci(0x4,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,MBR,0xBE1AFDFA,0x3F,0xFBFC1)/\EFI\BOOT\BOOTX64.efi"
 /// it will give "PciRoot(0x0)/Pci(0x4,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,MBR,0xBE1AFDFA,0x3F,0xFBFC1)"
@@ -37,7 +43,7 @@ pub fn device_path_root(path: &DevicePath) -> Result<String> {
             let item = item.to_string(DisplayOnly(false), AllowShortcuts(false));
             if item
                 .as_ref()
-                .map(|item| item.to_string().contains("("))
+                .map(|item| cstring16_contains_char(item, '('))
                 .unwrap_or(false)
             {
                 Some(item.unwrap_or_default())
@@ -62,7 +68,7 @@ pub fn device_path_subpath(path: &DevicePath) -> Result<String> {
             let item = item.to_string(DisplayOnly(false), AllowShortcuts(false));
             if item
                 .as_ref()
-                .map(|item| item.to_string().contains("("))
+                .map(|item| cstring16_contains_char(item, '('))
                 .unwrap_or(false)
             {
                 None
@@ -104,11 +110,11 @@ pub fn resolve_path(default_root_path: &DevicePath, input: &str) -> Result<Resol
             it.to_string(DisplayOnly(false), AllowShortcuts(false))
                 .unwrap_or_default()
         })
-        .map(|it| it.to_string().contains("("))
+        .map(|it| it.to_string().contains('('))
         .unwrap_or(false);
     if !path_has_device {
         let mut input = input.to_string();
-        if !input.starts_with("\\") {
+        if !input.starts_with('\\') {
             input.insert(0, '\\');
         }
         input.insert_str(

src/utils/framebuffer.rs

@@ -13,17 +13,33 @@ pub struct Framebuffer {
 
 impl Framebuffer {
     /// Creates a new framebuffer of the specified `width` and `height`.
-    pub fn new(width: usize, height: usize) -> Self {
+    pub fn new(width: usize, height: usize) -> Result<Self> {
-        Framebuffer {
+        // Verify that the size is valid during multiplication.
+        let size = width
+            .checked_mul(height)
+            .context("framebuffer size overflow")?;
+
+        // Initialize the pixel buffer with black pixels, with the verified size.
+        let pixels = vec![BltPixel::new(0, 0, 0); size];
+
+        Ok(Framebuffer {
             width,
             height,
-            pixels: vec![BltPixel::new(0, 0, 0); width * height],
+            pixels,
-        }
+        })
     }
 
     /// Mutably acquires a pixel of the framebuffer at the specified `x` and `y` coordinate.
     pub fn pixel(&mut self, x: usize, y: usize) -> Option<&mut BltPixel> {
-        self.pixels.get_mut(y * self.width + x)
+        // Verify that the coordinates are within the bounds of the framebuffer.
+        if x >= self.width || y >= self.height {
+            return None;
+        }
+
+        // Calculate the index of the pixel safely, returning None if it overflows.
+        let index = y.checked_mul(self.width)?.checked_add(x)?;
+        // Return the pixel at the index. If the index is out of bounds, this will return None.
+        self.pixels.get_mut(index)
     }
 
     /// Blit the framebuffer to the specified `gop` [GraphicsOutput].

src/utils/media_loader.rs

@@ -97,7 +97,7 @@ impl MediaLoaderHandle {
     }
 
     /// Creates a new device path for the media loader based on a vendor `guid`.
-    fn device_path(guid: Guid) -> Box<DevicePath> {
+    fn device_path(guid: Guid) -> Result<Box<DevicePath>> {
         // The buffer for the device path.
         let mut path = Vec::new();
         // Build a device path for the media loader with a vendor-specific guid.
@@ -106,18 +106,18 @@ impl MediaLoaderHandle {
                 vendor_guid: guid,
                 vendor_defined_data: &[],
             })
-            .unwrap() // We know that the device path is valid, so we can unwrap.
+            .context("unable to produce device path")?
             .finalize()
-            .unwrap(); // We know that the device path is valid, so we can unwrap.
+            .context("unable to produce device path")?;
         // Convert the device path to a boxed device path.
         // This is safer than dealing with a pooled device path.
-        path.to_boxed()
+        Ok(path.to_boxed())
     }
 
     /// Checks if the media loader is already registered with the UEFI stack.
     fn already_registered(guid: Guid) -> Result<bool> {
         // Acquire the device path for the media loader.
-        let path = Self::device_path(guid);
+        let path = Self::device_path(guid)?;
 
         let mut existing_path = path.as_ref();
 
@@ -142,7 +142,7 @@ impl MediaLoaderHandle {
     /// to load the data from.
     pub fn register(guid: Guid, data: Box<[u8]>) -> Result<MediaLoaderHandle> {
         // Acquire the vendor device path for the media loader.
-        let path = Self::device_path(guid);
+        let path = Self::device_path(guid)?;
 
         // Check if the media loader is already registered.
         // If it is, we can't register it again safely.