[DWARF] Improve logging when loading fails, remove extra bv creation

2 files changed, 93 insertions, 139 deletions

diff --git a/plugins/dwarf/dwarf_import/src/helpers.rs b/plugins/dwarf/dwarf_import/src/helpers.rs
index 8ef51f2f..33a78229 100644
--- a/plugins/dwarf/dwarf_import/src/helpers.rs
+++ b/plugins/dwarf/dwarf_import/src/helpers.rs
@@ -14,16 +14,14 @@
 
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
-use std::{ops::Deref, str::FromStr, sync::mpsc};
+use std::{str::FromStr, sync::mpsc};
 
 use crate::{DebugInfoBuilderContext, ReaderType};
 use binaryninja::binary_view::BinaryViewBase;
-use binaryninja::file_metadata::FileMetadata;
 use binaryninja::Endianness;
 use binaryninja::{
     binary_view::{BinaryView, BinaryViewExt},
     download::{DownloadInstanceInputOutputCallbacks, DownloadProvider},
-    rc::Ref,
     settings::Settings,
 };
 use gimli::Dwarf;
@@ -381,8 +379,8 @@ pub(crate) fn get_expr_value<R: ReaderType>(unit: &Unit<R>, attr: Attribute<R>)
     }
 }
 
-pub(crate) fn get_build_id(view: &BinaryView) -> Result<String, String> {
-    let mut build_id: Option<String> = None;
+pub(crate) fn get_build_id(view: &BinaryView) -> Result<Option<Vec<u8>>, String> {
+    let mut build_id: Option<Vec<u8>> = None;
 
     if let Some(raw_view) = view.raw_view() {
         if let Some(build_id_section) = raw_view.section_by_name(".note.gnu.build-id") {
@@ -432,21 +430,18 @@ pub(crate) fn get_build_id(view: &BinaryView) -> Result<String, String> {
             }
 
             let desc: &[u8] = &build_id_bytes[(12 + name_len as usize)..expected_len];
-            build_id = Some(desc.iter().map(|b| format!("{:02x}", b)).collect());
+            build_id = Some(desc.to_vec());
         }
     }
 
-    if let Some(x) = build_id {
-        Ok(x)
-    } else {
-        Err("Failed to get build id".to_string())
-    }
+    Ok(build_id)
 }
 
 pub(crate) fn download_debug_info(
-    build_id: &str,
+    build_id: &[u8],
     view: &BinaryView,
-) -> Result<Ref<BinaryView>, String> {
+) -> Result<Option<Vec<u8>>, String> {
+    let build_id_hex: String = build_id.iter().map(|x| format!("{:02x}", x)).collect();
     let mut settings_query_opts = QueryOptions::new_with_view(view);
     let settings = Settings::new();
     let debug_server_urls =
@@ -456,7 +451,7 @@ pub(crate) fn download_debug_info(
         let artifact_url = format!(
             "{}/buildid/{}/debuginfo",
             debug_server_url.trim_end_matches("/"),
-            build_id
+            build_id_hex
         );
 
         // Download from remote
@@ -510,15 +505,9 @@ pub(crate) fn download_debug_info(
                 ));
             }
         }
-
-        let options = "{\"analysis.debugInfo.internal\": false}";
-        let bv = BinaryView::from_data(FileMetadata::new().deref(), &data)
-            .map_err(|_| "Unable to create binary view from downloaded data".to_string())?;
-
-        return binaryninja::load_view(bv.deref(), false, Some(options))
-            .ok_or("Unable to load binary view from downloaded data".to_string());
+        return Ok(Some(data));
     }
-    Err("Could not find a server with debug info for this file".to_string())
+    Ok(None)
 }
 
 pub(crate) fn find_local_debug_file_from_path(path: &PathBuf, view: &BinaryView) -> Option<String> {
@@ -557,13 +546,15 @@ pub(crate) fn find_local_debug_file_from_path(path: &PathBuf, view: &BinaryView)
 }
 
 pub(crate) fn find_local_debug_file_for_build_id(
-    build_id: &str,
+    build_id: &[u8],
     view: &BinaryView,
 ) -> Option<String> {
-    let debug_ext_path = PathBuf::from(&build_id[..2]).join(format!("{}.debug", &build_id[2..]));
+    let build_id_hex: String = build_id.iter().map(|x| format!("{:02x}", x)).collect();
+    let debug_ext_path =
+        PathBuf::from(&build_id_hex[..2]).join(format!("{}.debug", &build_id_hex[2..]));
 
-    let elf_path = PathBuf::from(&build_id[..2])
-        .join(&build_id[2..])
+    let elf_path = PathBuf::from(&build_id_hex[..2])
+        .join(&build_id_hex[2..])
         .join("elf");
 
     find_local_debug_file_from_path(&debug_ext_path, view)
@@ -571,24 +562,19 @@ pub(crate) fn find_local_debug_file_for_build_id(
 }
 
 pub(crate) fn load_debug_info_for_build_id(
-    build_id: &str,
+    build_id: &[u8],
     view: &BinaryView,
-) -> (Option<Ref<BinaryView>>, bool) {
+) -> Result<Option<Vec<u8>>, String> {
     let mut settings_query_opts = QueryOptions::new_with_view(view);
     let settings = Settings::new();
     if let Some(debug_file_path) = find_local_debug_file_for_build_id(build_id, view) {
-        return (
-            binaryninja::load_with_options(
-                debug_file_path,
-                false,
-                Some("{\"analysis.debugInfo.internal\": false}"),
-            ),
-            true,
-        );
+        return std::fs::read(&debug_file_path)
+            .map(|x| Some(x))
+            .map_err(|e| format!("Failed to read local debug file {}: {}", debug_file_path, e));
     } else if settings.get_bool_with_opts("network.enableDebuginfod", &mut settings_query_opts) {
-        return (download_debug_info(build_id, view).ok(), true);
+        return download_debug_info(build_id, view);
     }
-    (None, false)
+    Ok(None)
 }
 
 pub(crate) fn find_sibling_debug_file(view: &BinaryView) -> Option<String> {
@@ -650,21 +636,12 @@ pub(crate) fn find_sibling_debug_file(view: &BinaryView) -> Option<String> {
     None
 }
 
-pub(crate) fn load_sibling_debug_file(view: &BinaryView) -> (Option<Ref<BinaryView>>, bool) {
+pub(crate) fn load_sibling_debug_file(view: &BinaryView) -> Result<Option<Vec<u8>>, String> {
     let Some(debug_file) = find_sibling_debug_file(view) else {
-        return (None, false);
-    };
-
-    let load_settings = match view.default_platform() {
-        Some(plat) => format!(
-            "{{\"analysis.debugInfo.internal\": false, \"loader.platform\": \"{}\"}}",
-            plat.name()
-        ),
-        None => "{\"analysis.debugInfo.internal\": false}".to_string(),
+        return Ok(None);
     };
 
-    (
-        binaryninja::load_with_options(debug_file, false, Some(load_settings)),
-        true,
-    )
+    std::fs::read(&debug_file)
+        .map(|x| Some(x))
+        .map_err(|e| format!("Failed to read sibling debug file {}: {}", debug_file, e))
 }
diff --git a/plugins/dwarf/dwarf_import/src/lib.rs b/plugins/dwarf/dwarf_import/src/lib.rs
index 838a3612..41229a9f 100644
--- a/plugins/dwarf/dwarf_import/src/lib.rs
+++ b/plugins/dwarf/dwarf_import/src/lib.rs
@@ -20,7 +20,6 @@ mod types;
 
 use std::collections::HashMap;
 use std::path::PathBuf;
-use std::str::FromStr;
 
 use crate::dwarfdebuginfo::{DebugInfoBuilder, DebugInfoBuilderContext};
 use crate::functions::parse_function_entry;
@@ -499,44 +498,6 @@ where
     }
 }
 
-fn get_supplementary_build_id(bv: &BinaryView) -> Option<String> {
-    let raw_view = bv.raw_view()?;
-    if let Some(section) = raw_view.section_by_name(".gnu_debugaltlink") {
-        let start = section.start();
-        let len = section.len();
-
-        if len < 20 {
-            // Not large enough to hold a build id
-            return None;
-        }
-
-        raw_view
-            .read_vec(start, len)
-            .splitn(2, |x| *x == 0)
-            .last()
-            .map(|a| a.iter().map(|b| format!("{:02x}", b)).collect())
-    } else {
-        None
-    }
-}
-
-fn get_supplementary_file_path(bv: &BinaryView) -> Option<PathBuf> {
-    let raw_view = bv.raw_view()?;
-    if let Some(section) = raw_view.section_by_name(".gnu_debugaltlink") {
-        let start = section.start();
-        let len = section.len();
-
-        raw_view
-            .read_vec(start, len)
-            .splitn(2, |x| *x == 0)
-            .next()
-            .and_then(|a| String::from_utf8(a.to_vec()).ok())
-            .and_then(|p| PathBuf::from_str(&p).ok())
-    } else {
-        None
-    }
-}
-
 fn parse_range_data_offsets(file: &object::File) -> Result<IntervalMap<u64, i64>, String> {
     let dwo_file = file.section_by_name(".debug_info.dwo").is_some();
     let endian = match file.endianness() {
@@ -697,7 +658,7 @@ impl CustomDebugInfoParser for DWARFParser {
             return true;
         }
         if dwarfreader::has_build_id_section(view) {
-            if let Ok(build_id) = get_build_id(view) {
+            if let Ok(Some(build_id)) = get_build_id(view) {
                 if helpers::find_local_debug_file_for_build_id(&build_id, view).is_some() {
                     return true;
                 }
@@ -713,30 +674,46 @@ impl CustomDebugInfoParser for DWARFParser {
         &self,
         debug_info: &mut DebugInfo,
         bv: &BinaryView,
-        debug_file: &BinaryView,
+        debug_bv: &BinaryView,
         progress: Box<dyn Fn(usize, usize) -> Result<(), ()>>,
     ) -> bool {
-        let (external_file, close_external) = if !dwarfreader::is_valid(bv) {
-            if let (Some(debug_view), x) = helpers::load_sibling_debug_file(bv) {
-                (Some(debug_view), x)
-            } else if let Ok(build_id) = get_build_id(bv) {
-                load_debug_info_for_build_id(&build_id, bv)
-            } else {
-                (None, false)
-            }
-        } else {
-            (None, false)
-        };
-
-        let debug_bv = external_file.as_deref().unwrap_or(debug_file);
-
-        // Read the raw view to an object::File so relocations get handled for us
-        let Some(raw_view) = debug_bv.raw_view() else {
-            log::error!("Failed to get raw view for debug bv");
+        let Some(debug_data_vec) = dwarfreader::is_valid(debug_bv)
+            .then(|| {
+                // Load the raw view of the debug bv passed in if it has valid debug info
+                let raw_view = debug_bv.raw_view().expect("Failed to get raw view");
+                raw_view.read_vec(0, raw_view.len() as usize)
+            })
+            .or_else(|| {
+                // Try loading sibling debug files
+                match helpers::load_sibling_debug_file(bv) {
+                    Ok(x) => x,
+                    Err(e) => {
+                        log::error!("Failed loading sibling debug file: {}", e);
+                        None
+                    }
+                }
+            })
+            .or_else(|| {
+                // Try loading from the file's build id
+                if let Ok(Some(build_id)) = get_build_id(bv) {
+                    match load_debug_info_for_build_id(&build_id, bv) {
+                        Ok(x) => x,
+                        Err(e) => {
+                            log::error!("Failed loading debug info from build id: {}", e);
+                            None
+                        }
+                    }
+                } else {
+                    // No build id found
+                    None
+                }
+            })
+        else {
+            // There isn't any dwarf info available to load
             return false;
         };
-        let raw_view_data = raw_view.read_vec(0, raw_view.len() as usize);
-        let debug_file = match object::File::parse(&*raw_view_data) {
+
+        let debug_file = match object::File::parse(debug_data_vec.as_slice()) {
             Ok(x) => x,
             Err(e) => {
                 log::error!("Failed to parse bv: {}", e);
@@ -746,31 +723,35 @@ impl CustomDebugInfoParser for DWARFParser {
 
         // TODO: allow passing a supplementary file path as a setting?
         // Try to load supplementary file from build id, falling back to file path
-        let sup_view_data = get_supplementary_build_id(debug_bv)
-            .and_then(|build_id| {
-                load_debug_info_for_build_id(&build_id, bv)
-                    .0
-                    .map(|x| x.raw_view().expect("Failed to get raw view"))
-            })
-            .or_else(|| {
-                get_supplementary_file_path(debug_bv).and_then(|sup_file_path_suggestion| {
-                    find_local_debug_file_from_path(&sup_file_path_suggestion, debug_bv).and_then(
-                        |sup_file_path| {
-                            binaryninja::load_with_options(
-                                sup_file_path,
-                                false,
-                                Some("{\"analysis.debugInfo.internal\": false}"),
-                            )
+        let sup_view_data = debug_file.gnu_debugaltlink().ok().flatten().and_then(
+            |(sup_filename, sup_build_id)| {
+                // Try loading from build id
+                let sup_data = match load_debug_info_for_build_id(sup_build_id, bv) {
+                    Ok(x) => x,
+                    Err(e) => {
+                        log::error!("Failed to load supplementary debug file: {}", e);
+                        None
+                    }
+                };
+
+                // Try loading from file path if build id loading didn't work
+                sup_data.or_else(|| match std::str::from_utf8(sup_filename) {
+                    Ok(x) => find_local_debug_file_from_path(&PathBuf::from(x), bv).and_then(
+                        |sup_file_path| match std::fs::read(sup_file_path) {
+                            Ok(sup_data) => Some(sup_data),
+                            Err(e) => {
+                                log::error!("Failed reading supplementary file {}: {}", x, e);
+                                None
+                            }
                         },
-                    )
+                    ),
+                    Err(e) => {
+                        log::error!("Supplementary file path is invalid utf8: {}", e);
+                        None
+                    }
                 })
-            })
-            .and_then(|sup_bv| {
-                let sup_raw_view = sup_bv.raw_view()?;
-                let sup_raw_data = sup_raw_view.read_vec(0, sup_raw_view.len() as usize);
-                sup_raw_view.file().close();
-                Some(sup_raw_data)
-            });
+            },
+        );
 
         let sup_file =
             sup_view_data
@@ -815,10 +796,6 @@ impl CustomDebugInfoParser for DWARFParser {
             }
         };
 
-        if let (Some(ext), true) = (external_file, close_external) {
-            ext.file().close();
-        }
-
         result
     }
 }