Swallow build ID read failures

So far any failures to read the build ID as part of an address
normalization request have short-circuit the entire operation. While
somewhat reasonable behavior in principle, the result will be that such
a read failure for one of the files can fail normalization for others,
which isn't really behavior that we are striving for. Rather, because we
support batched symbolization, we should report the failure at the
level of an individual address.
For build IDs specifically, we already have to work with files not
containing a build ID and so the most sensible behavior is to just map
read failures to a non-existent build ID.

Signed-off-by: Daniel Müller <deso@posteo.net>

CHANGELOG.md

@@ -1,3 +1,9 @@
+Unreleased
+----------
+- Adjusted normalization logic to not fail overall operation on build ID
+  read failure
+
+
 0.2.0-rc.0
 ----------
 - Added support for transparently following debug links in ELF binaries

capi/include/blazesym.h

@@ -305,6 +305,9 @@ typedef struct blaze_normalizer_opts {
   /**
    * Whether to read and report build IDs as part of the normalization
    * process.
+   *
+   * Note that build ID read failures will be swallowed without
+   * failing the normalization operation.
    */
   bool build_ids;
   /**
@@ -347,7 +350,7 @@ typedef struct blaze_user_meta_elf {
    */
   size_t build_id_len;
   /**
-   * The optional build ID of the ELF file, if found.
+   * The optional build ID of the ELF file, if found and readable.
    */
   uint8_t *build_id;
   /**

capi/src/normalize.rs

@@ -51,6 +51,9 @@ pub struct blaze_normalizer_opts {
     pub cache_maps: bool,
     /// Whether to read and report build IDs as part of the normalization
     /// process.
+    ///
+    /// Note that build ID read failures will be swallowed without
+    /// failing the normalization operation.
     pub build_ids: bool,
     /// Whether or not to cache build IDs. This flag only has an effect
     /// if build ID reading is enabled in the first place.
@@ -292,7 +295,7 @@ pub struct blaze_user_meta_elf {
     pub path: *mut c_char,
     /// The length of the build ID, in bytes.
     pub build_id_len: usize,
-    /// The optional build ID of the ELF file, if found.
+    /// The optional build ID of the ELF file, if found and readable.
     pub build_id: *mut u8,
     /// Unused member available for future expansion.
     pub reserved: [u8; 8],

src/normalize/buildid.rs

@@ -99,16 +99,30 @@ fn read_build_id_impl(parser: &ElfParser) -> Result<Option<BuildId>> {
 }
 
 pub(super) trait BuildIdReader<'src> {
-    fn read_build_id(&self, path: &Path) -> Result<Option<BuildId<'src>>>;
+    fn read_build_id(&self, path: &Path) -> Option<BuildId<'src>> {
+        #[cfg_attr(
+            feature = "tracing",
+            crate::log::instrument(err, skip_all, fields(path = ?path))
+        )]
+        fn read_build_id<'src, B>(slf: &B, path: &Path) -> Result<Option<BuildId<'src>>>
+        where
+            B: BuildIdReader<'src> + ?Sized,
+        {
+            slf.read_build_id_fallible(path)
+        }
+
+        read_build_id(self, path).ok().flatten()
+    }
+
+    fn read_build_id_fallible(&self, path: &Path) -> Result<Option<BuildId<'src>>>;
 }
 
 
 pub(super) struct DefaultBuildIdReader;
 
 impl BuildIdReader<'_> for DefaultBuildIdReader {
     /// Attempt to read an ELF binary's build ID from a file.
-    #[cfg_attr(feature = "tracing", crate::log::instrument(skip(self)))]
-    fn read_build_id(&self, path: &Path) -> Result<Option<BuildId<'static>>> {
+    fn read_build_id_fallible(&self, path: &Path) -> Result<Option<BuildId<'static>>> {
         let parser = ElfParser::open(path)?;
         let buildid = read_build_id_impl(&parser)?.map(|buildid| Cow::Owned(buildid.to_vec()));
         Ok(buildid)
@@ -130,8 +144,7 @@ impl<'cache> CachingBuildIdReader<'cache> {
 
 impl<'src> BuildIdReader<'src> for CachingBuildIdReader<'src> {
     /// Attempt to read an ELF binary's build ID from a file.
-    #[cfg_attr(feature = "tracing", crate::log::instrument(skip(self)))]
-    fn read_build_id(&self, path: &Path) -> Result<Option<BuildId<'src>>> {
+    fn read_build_id_fallible(&self, path: &Path) -> Result<Option<BuildId<'src>>> {
         let (file, cell) = self.cache.entry(path)?;
         let build_id = cell
             .get_or_try_init(|| {
@@ -151,7 +164,7 @@ pub(super) struct NoBuildIdReader;
 
 impl BuildIdReader<'_> for NoBuildIdReader {
     #[inline]
-    fn read_build_id(&self, _path: &Path) -> Result<Option<BuildId<'static>>> {
+    fn read_build_id_fallible(&self, _path: &Path) -> Result<Option<BuildId<'static>>> {
         Ok(None)
     }
 }

src/normalize/meta.rs

@@ -54,7 +54,7 @@ pub struct Apk {
 pub struct Elf<'src> {
     /// The canonical absolute path to the ELF file, including its name.
     pub path: PathBuf,
-    /// The ELF file's build ID, if available.
+    /// The ELF file's build ID, if available and readable.
     pub build_id: Option<BuildId<'src>>,
     /// The struct is non-exhaustive and open to extension.
     #[doc(hidden)]

src/normalize/normalizer.rs

@@ -59,6 +59,9 @@ pub struct Builder {
     cache_maps: bool,
     /// Whether to read and report build IDs as part of the normalization
     /// process.
+    ///
+    /// Note that build ID read failures will be swallowed without
+    /// failing the normalization operation.
     build_ids: bool,
     /// Whether or not to cache build IDs. This flag only has an effect
     /// if build ID reading is enabled in the first place.
@@ -142,6 +145,9 @@ pub struct Normalizer {
     cache_maps: bool,
     /// Flag indicating whether or not to read build IDs as part of the
     /// normalization process.
+    ///
+    /// Note that build ID read failures will be swallowed without
+    /// failing the normalization operation.
     build_ids: bool,
     /// Whether or not to cache build IDs. This flag only has an effect
     /// if build ID reading is enabled in the first place.

src/normalize/user.rs

@@ -29,7 +29,7 @@ fn make_elf_meta<'src>(
 ) -> Result<UserMeta<'src>> {
     let elf = Elf {
         path: path.to_path_buf(),
-        build_id: build_id_reader.read_build_id(maps_file)?,
+        build_id: build_id_reader.read_build_id(maps_file),
         _non_exhaustive: (),
     };
     let meta = UserMeta::Elf(elf);
@@ -272,6 +272,8 @@ mod tests {
     use test_log::test;
 
     use crate::normalize::buildid::NoBuildIdReader;
+    use crate::BuildId;
+    use crate::Error;
     use crate::Pid;
 
 
@@ -337,4 +339,39 @@ mod tests {
         test(0x7fffffff1001, Reason::Unmapped);
         test(0x7fffffffffff, Reason::Unmapped);
     }
+
+    struct FailingBuildIdReader;
+
+    impl BuildIdReader<'_> for FailingBuildIdReader {
+        #[inline]
+        fn read_build_id_fallible(&self, _path: &Path) -> Result<Option<BuildId<'static>>> {
+            Err(Error::with_unsupported("induced failure"))
+        }
+    }
+
+    /// Check that errors when reading build IDs are handled gracefully.
+    #[test]
+    fn build_id_read_failures() {
+        let addrs = [build_id_read_failures as Addr];
+        let map_files = false;
+
+        let entries = maps::parse_filtered(Pid::Slf).unwrap();
+        let reader = FailingBuildIdReader;
+        let mut handler = NormalizationHandler::new(&reader, addrs.len(), map_files);
+        let () = normalize_sorted_user_addrs_with_entries(
+            addrs.as_slice().iter().copied(),
+            entries,
+            &mut handler,
+        )
+        .unwrap();
+
+        let normalized = handler.normalized;
+        assert_eq!(normalized.outputs.len(), 1);
+        assert_eq!(normalized.meta.len(), 1);
+        assert!(
+            normalized.meta[0].elf().is_some(),
+            "{:?}",
+            normalized.meta[0]
+        );
+    }
 }