Review fixes part 3 plus clippy lint fixes

libclamav_rust/src/css_image_extract.rs

@@ -152,7 +152,7 @@ impl<'a> CssImageExtractor<'a> {
             };
 
             // Trim off " at end.
-            let c = url_parameter.graphemes(true).rev().next();
+            let c = url_parameter.graphemes(true).next_back();
             if let Some(c) = c {
                 if c == "\"" {
                     (url_parameter, _) = url_parameter.split_at(url_parameter.len() - 1);

libclamav_rust/src/ctx.rs

@@ -21,13 +21,14 @@
  */
 
 use std::convert::TryInto;
+use std::path::{PathBuf};
 use std::slice;
 
 use thiserror;
 
 use crate::fmap::FMap;
 use crate::sys::cli_ctx;
-use crate::util::optional_cstring;
+use crate::util::str_from_ptr;
 
 /// Error enumerates all possible errors returned by this library.
 #[derive(thiserror::Error, Debug)]
@@ -46,40 +47,47 @@ pub enum Error {
 
     #[error("Invalid FMap")]
     BadMap,
+
+    #[error("File path is not valid UTF8")]
+    FilepathNotUtf8,
 }
 
 /// Get the ctx.sub_filepath as an Option<'str>
 ///
 /// # Safety
 ///
 /// Must be a valid ctx pointer.
-pub fn sub_filepath(ctx: *mut cli_ctx) -> Result<Option<&'static str>, Error> {
+pub unsafe fn sub_filepath(ctx: *mut cli_ctx) -> Result<Option<PathBuf>, Error> {
     if ctx.is_null() {
         return Err(Error::NullPointer("ctx"));
     }
 
-    Ok(optional_cstring(unsafe { *ctx }.sub_filepath))
+    Ok(str_from_ptr(unsafe { *ctx }.sub_filepath)
+        .map_err(|_| Error::FilepathNotUtf8)?
+        .map(PathBuf::from))
 }
 
 /// Get the ctx.target_filepath as an Option<'str>
 ///
 /// # Safety
 ///
 /// Must be a valid ctx pointer.
-pub fn target_filepath(ctx: *mut cli_ctx) -> Result<Option<&'static str>, Error> {
+pub unsafe fn target_filepath(ctx: *mut cli_ctx) -> Result<Option<PathBuf>, Error> {
     if ctx.is_null() {
         return Err(Error::NullPointer("ctx"));
     }
 
-    Ok(optional_cstring(unsafe { *ctx }.target_filepath))
+    Ok(str_from_ptr(unsafe { *ctx }.target_filepath)
+        .map_err(|_| Error::FilepathNotUtf8)?
+        .map(PathBuf::from))
 }
 
 /// Get the fmap for the current layer.
 ///
 /// # Safety
 ///
 /// Must be a valid ctx pointer.
-pub fn current_fmap(ctx: *mut cli_ctx) -> Result<FMap, Error> {
+pub unsafe fn current_fmap(ctx: *mut cli_ctx) -> Result<FMap, Error> {
     if ctx.is_null() {
         return Err(Error::NullPointer("ctx"));
     }

libclamav_rust/src/fmap.rs

@@ -25,7 +25,7 @@ use std::convert::TryFrom;
 use log::{debug, error};
 use thiserror::Error;
 
-use crate::{sys, util::optional_cstring};
+use crate::{sys, util::str_from_ptr};
 
 /// FMapError enumerates all possible errors returned by this library.
 #[derive(Error, Debug)]
@@ -71,7 +71,7 @@ impl<'a> FMap {
 
         let ptr: *const u8 = unsafe { need_fn(self.fmap_ptr, at, len, 1) } as *const u8;
 
-        if ptr == std::ptr::null() {
+        if ptr.is_null() {
             let fmap_size = unsafe { *self.fmap_ptr }.len;
             debug!(
                 "need_off at {:?} len {:?} for fmap size {:?} returned NULL",
@@ -96,7 +96,15 @@ impl<'a> FMap {
         unsafe { (*self.fmap_ptr).len }
     }
 
+    pub fn is_empty(&self) -> bool {
+        unsafe { (*self.fmap_ptr).len == 0 }
+    }
+
     pub fn name(&self) -> &'static str {
-        optional_cstring(unsafe { (*self.fmap_ptr).name }).unwrap_or("<unnamed>")
+        let name_result = unsafe { str_from_ptr((*self.fmap_ptr).name) };
+        match name_result {
+            Ok(Some(name)) => name,
+            _ => "",
+        }
     }
 }

libclamav_rust/src/onenote.rs

@@ -55,8 +55,8 @@ pub enum Error {
     #[error("Unable to parse OneNote file")]
     Parse,
 
-    #[error("Failed to load image due to bug in image decoder")]
-    SectionParsePanic(),
+    #[error("Failed to parse OneNote file due to a panic in the onenote_parser library")]
+    OneNoteParserPanic,
 }
 
 fn find_bytes(haystack: &[u8], needle: &[u8]) -> Option<usize> {
@@ -137,7 +137,7 @@ impl<'a> OneNote<'a> {
                                                 };
 
                                                 embedded_files.push(ExtractedFile {
-                                                    name: name,
+                                                    name,
                                                     data: data.to_vec(),
                                                 });
                                             }
@@ -158,11 +158,11 @@ impl<'a> OneNote<'a> {
         // Try to parse the section buffer using the onenote_parser crate.
         // Attempt to catch panics in case the parser encounter unexpected issues.
         let result_result = panic::catch_unwind(|| -> Result<Vec<ExtractedFile>, Error> {
-            Ok(parse_section_buffer(data, filename)?)
+            parse_section_buffer(data, filename)
         });
 
         // Check if it panicked. If no panic, grab the parse result.
-        let result = result_result.map_err(|_| Error::Parse)?;
+        let result = result_result.map_err(|_| Error::OneNoteParserPanic)?;
 
         if let Ok(embedded_files) = result {
             // Successfully parsed the OneNote file with the onenote_parser crate.
@@ -188,6 +188,7 @@ impl<'a> OneNote<'a> {
         }
     }
 
+    /// Open a OneNote document given the document was provided as a slice of bytes.
     pub fn next_file(&mut self) -> Option<ExtractedFile> {
         debug!("Looking to extract file from OneNote section...");
 
@@ -208,24 +209,22 @@ impl<'a> OneNote<'a> {
                 | ((data_length_bytes[1] as u64) << 8)
                 | (data_length_bytes[0] as u64);
 
-            let data: &[u8] = &remaining[SIZE_OF_FILE_DATA_HEADER
-                ..SIZE_OF_FILE_DATA_HEADER + data_length as usize];
+            let data: &[u8] = &remaining
+                [SIZE_OF_FILE_DATA_HEADER..SIZE_OF_FILE_DATA_HEADER + data_length as usize];
 
             file_data = Some(data.to_vec());
 
-            Some(
-                &remaining
-                    [SIZE_OF_FILE_DATA_HEADER + (data_length as usize)..remaining.len()],
-            )
+            Some(&remaining[SIZE_OF_FILE_DATA_HEADER + (data_length as usize)..remaining.len()])
         } else {
             None
         };
 
         self.remaining = remaining;
 
-        file_data.map(|data| ExtractedFile { data, name: None, })
+        file_data.map(|data| ExtractedFile { data, name: None })
     }
 
+    /// Get the next file from the OneNote document using the method required for when we've read the file into a Vec.
     pub fn next_file_vec(&mut self) -> Option<ExtractedFile> {
         debug!("Looking to extract file from OneNote section...");
 
@@ -246,22 +245,22 @@ impl<'a> OneNote<'a> {
                 | ((data_length_bytes[1] as u64) << 8)
                 | (data_length_bytes[0] as u64);
 
-            let data: &[u8] = &remaining[SIZE_OF_FILE_DATA_HEADER
-                ..SIZE_OF_FILE_DATA_HEADER + data_length as usize];
+            let data: &[u8] = &remaining
+                [SIZE_OF_FILE_DATA_HEADER..SIZE_OF_FILE_DATA_HEADER + data_length as usize];
 
             file_data = Some(data.to_vec());
 
             Some(Vec::from(
-                &remaining
-                    [SIZE_OF_FILE_DATA_HEADER + (data_length as usize)..remaining.len()],
+                &remaining[SIZE_OF_FILE_DATA_HEADER + (data_length as usize)..remaining.len()],
             ))
         } else {
             None
         };
 
-        file_data.map(|data| ExtractedFile { data, name: None, })
+        file_data.map(|data| ExtractedFile { data, name: None })
     }
 
+    /// Get the next file from the OneNote document using the method required for the onenote_parser crate.
     pub fn next_file_parser(&mut self) -> Option<ExtractedFile> {
         self.embedded_files.pop()
     }
@@ -278,7 +277,7 @@ impl<'a> Iterator for OneNote<'a> {
         } else if self.remaining_vec.is_some() {
             // Data stored in a Vec.
             self.next_file_vec()
-        } else if self.embedded_files.len() > 0 {
+        } else if !self.embedded_files.is_empty() {
             // Data stored in a Vec.
             self.next_file_parser()
         } else {

libclamav_rust/src/scanners.rs

@@ -66,18 +66,22 @@ fn magic_scan(ctx: *mut cli_ctx, buf: &[u8], name: Option<String>) -> cl_error_t
     }
 
     // Okay now safe to drop the name CString.
-    drop(unsafe { CString::from_raw(name_ptr) });
+    let _ = unsafe { CString::from_raw(name_ptr) };
 
     ret
 }
 
 /// Scan a OneNote file for attachments
+/// 
+/// # Safety
+/// 
+/// Must be a valid ctx pointer.
 #[no_mangle]
 pub unsafe extern "C" fn scan_onenote(ctx: *mut cli_ctx) -> cl_error_t {
     let fmap = match ctx::current_fmap(ctx) {
         Ok(fmap) => fmap,
         Err(e) => {
-            warn!("Error getting FMap from ctx: {}", e);
+            warn!("Error getting FMap from ctx: {e}");
             return cl_error_t_CL_ERROR;
         }
     };
@@ -86,33 +90,29 @@ pub unsafe extern "C" fn scan_onenote(ctx: *mut cli_ctx) -> cl_error_t {
         Ok(bytes) => bytes,
         Err(err) => {
             error!(
-                "Failed to get file bytes for fmap of size {}: {}",
-                fmap.len(),
-                err.to_string()
+                "Failed to get file bytes for fmap of size {}: {err}",
+                fmap.len()
             );
             return cl_error_t_CL_ERROR;
         }
     };
 
-    let one = OneNote::from_bytes(file_bytes, &Path::new(fmap.name()));
-
-    let one = match one {
+    let one = match OneNote::from_bytes(file_bytes, Path::new(fmap.name())) {
         Ok(x) => x,
         Err(err) => {
             error!("Failed to parse OneNote file: {}", err.to_string());
             return cl_error_t_CL_ERROR;
         }
     };
 
-    one.into_iter().enumerate().all(|(_, attachment)| {
+    one.into_iter().all(|attachment| {
         debug!(
             "Extracted {}-byte attachment with name: {:?}",
             attachment.data.len(),
             attachment.name
         );
 
         let ret = magic_scan(ctx, &attachment.data, attachment.name);
-
         if ret != cl_error_t_CL_SUCCESS {
             warn!("cl_scandesc_magic returned error: {}", ret);
             return false;

libclamav_rust/src/util.rs

@@ -20,7 +20,6 @@
  *  MA 02110-1301, USA.
  */
 
-use log::warn;
 use std::ffi::CStr;
 use std::fs::File;
 
@@ -55,16 +54,12 @@ pub fn file_from_fd_or_handle(fd: i32) -> File {
 ///
 /// The caller is responsible for making sure the lifetime of the pointer
 /// exceeds the lifetime of the output string.
-pub fn optional_cstring(ptr: *const i8) -> Option<&'static str> {
+/// 
+/// ptr must be a valid pointer to a C string.
+pub unsafe fn str_from_ptr(ptr: *const i8) -> Result<Option<&'static str>, std::str::Utf8Error> {
     if ptr.is_null() {
-        return None;
+        return Ok(None);
     }
 
-    match unsafe { CStr::from_ptr(ptr) }.to_str() {
-        Err(e) => {
-            warn!("{} is not valid unicode: {}", stringify!(ptr), e);
-            return None;
-        }
-        Ok(s) => Some(s),
-    }
+    Some(unsafe { CStr::from_ptr(ptr) }.to_str()).transpose()
 }