fix: Address code review feedback for Mach-O load command extraction (#103)

* Initial plan

* Apply code review fixes: error handling, integer overflow, formatting

Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: unclesp1d3r <251112+unclesp1d3r@users.noreply.github.com>

Author: Copilot

src/extraction/macho_load_commands.rs

@@ -6,27 +6,31 @@
 //!
 //! # Examples
 //!
-//! ```rust
+//! ```rust,no_run
+//! use std::error::Error;
 //! use stringy::extraction::macho_load_commands::extract_load_command_strings;
 //! use stringy::types::{Tag, StringSource};
 //!
-//! let macho_data = std::fs::read("example.dylib")?;
-//! let strings = extract_load_command_strings(&macho_data);
+//! fn main() -> Result<(), Box<dyn Error>> {
+//!     let macho_data = std::fs::read("example.dylib")?;
+//!     let strings = extract_load_command_strings(&macho_data);
 //!
-//! // Filter dylib paths
-//! let dylib_paths: Vec<_> = strings.iter()
-//!     .filter(|s| s.tags.contains(&Tag::DylibPath))
-//!     .collect();
+//!     // Filter dylib paths
+//!     let dylib_paths: Vec<_> = strings.iter()
+//!         .filter(|s| s.tags.contains(&Tag::DylibPath))
+//!         .collect();
 //!
-//! // Filter rpaths
-//! let rpaths: Vec<_> = strings.iter()
-//!     .filter(|s| s.tags.contains(&Tag::Rpath))
-//!     .collect();
+//!     // Filter rpaths
+//!     let rpaths: Vec<_> = strings.iter()
+//!         .filter(|s| s.tags.contains(&Tag::Rpath))
+//!         .collect();
 //!
-//! // Filter framework paths
-//! let framework_paths: Vec<_> = strings.iter()
-//!     .filter(|s| s.tags.contains(&Tag::FrameworkPath))
-//!     .collect();
+//!     // Filter framework paths
+//!     let framework_paths: Vec<_> = strings.iter()
+//!         .filter(|s| s.tags.contains(&Tag::FrameworkPath))
+//!         .collect();
+//!     Ok(())
+//! }
 //! ```
 
 use crate::types::{Encoding, FoundString, StringSource, Tag};
@@ -188,8 +192,12 @@ fn extract_architecture_data<'a>(
     let offset = arch.offset as usize;
     let size = arch.size as usize;
 
-    if offset + size <= data.len() {
-        Ok(&data[offset..offset + size])
+    if let Some(end) = offset.checked_add(size) {
+        if end <= data.len() {
+            Ok(&data[offset..end])
+        } else {
+            Err(())
+        }
     } else {
         Err(())
     }

tests/fixtures/README.md

@@ -5,7 +5,10 @@ This directory contains pre-compiled binary test fixtures used for snapshot test
 ## Fixtures
 
 - `test_binary_elf` - x86-64 ELF binary
-- `test_binary_macho` - ARM64 Mach-O binary (contains typical load commands including LC_LOAD_DYLIB for system library dependencies like libSystem.B.dylib, potentially LC_RPATH commands, and framework dependencies if any frameworks are linked)
+- `test_binary_macho` - ARM64 Mach-O binary with standard load commands:
+  - LC_LOAD_DYLIB for system library dependencies (e.g., libSystem.B.dylib)
+  - May include LC_RPATH commands
+  - May include framework dependencies
 - `test_binary_pe.exe` - x86-64 PE binary
 - `test_binary_with_resources.exe` - x86-64 PE binary with VERSIONINFO and STRINGTABLE resources
 

tests/integration_macho.rs

@@ -267,11 +267,12 @@ fn test_macho_load_command_extraction_snapshot() {
     for (i, string) in dylib_paths.iter().take(20).enumerate() {
         let is_framework = string.text.contains(".framework");
         output.push_str(&format!(
-            "Dylib Path {}: {} {}\n",
+            "Dylib Path {}: {}{}",
             i + 1,
             string.text,
-            if is_framework { "(Framework)" } else { "" }
+            if is_framework { " (Framework)" } else { "" }
         ));
+        output.push('\n');
     }
     if dylib_paths.len() > 20 {
         output.push_str(&format!("... and {} more\n", dylib_paths.len() - 20));
@@ -285,15 +286,16 @@ fn test_macho_load_command_extraction_snapshot() {
     for (i, string) in rpaths.iter().take(20).enumerate() {
         let has_variable = has_rpath_variable(&string.text);
         output.push_str(&format!(
-            "Rpath {}: {} {}\n",
+            "Rpath {}: {}{}",
             i + 1,
             string.text,
             if has_variable {
-                "(Contains @-variable)"
+                " (Contains @-variable)"
             } else {
                 ""
             }
         ));
+        output.push('\n');
     }
     if rpaths.len() > 20 {
         output.push_str(&format!("... and {} more\n", rpaths.len() - 20));
@@ -489,16 +491,14 @@ fn test_macho_rpath_variable_detection() {
 
     for rpath_var in &rpaths_with_vars {
         let mut variables_found = Vec::new();
-        if has_rpath_variable(&rpath_var.text) {
-            if rpath_var.text.contains("@rpath") {
-                variables_found.push("@rpath");
-            }
-            if rpath_var.text.contains("@executable_path") {
-                variables_found.push("@executable_path");
-            }
-            if rpath_var.text.contains("@loader_path") {
-                variables_found.push("@loader_path");
-            }
+        if rpath_var.text.contains("@rpath") {
+            variables_found.push("@rpath");
+        }
+        if rpath_var.text.contains("@executable_path") {
+            variables_found.push("@executable_path");
+        }
+        if rpath_var.text.contains("@loader_path") {
+            variables_found.push("@loader_path");
         }
         println!(
             "Rpath variable found: {} (variables: {:?})",

tests/snapshots/integration_macho__macho_load_command_strings.snap

@@ -1,13 +1,12 @@
 ---
 source: tests/integration_macho.rs
-assertion_line: 314
 expression: output
 ---
 === DYLIB PATHS ===
 Total: 2
 
-Dylib Path 1: /usr/lib/libSystem.B.dylib 
-Dylib Path 2: self 
+Dylib Path 1: /usr/lib/libSystem.B.dylib
+Dylib Path 2: self
 
 === RPATHS ===
 Total: 0