From 83a48db5d2d275b6b9994ae0967fb024984073b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Fri, 12 Jul 2024 14:29:10 -0700 Subject: [PATCH] Swallow build ID read failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 6 ++++++ capi/include/blazesym.h | 5 ++++- capi/src/normalize.rs | 5 ++++- src/normalize/buildid.rs | 25 ++++++++++++++++++------ src/normalize/meta.rs | 2 +- src/normalize/normalizer.rs | 6 ++++++ src/normalize/user.rs | 39 ++++++++++++++++++++++++++++++++++++- 7 files changed, 78 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d380ad2..9f18369a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/capi/include/blazesym.h b/capi/include/blazesym.h index b925d164..f377b890 100644 --- a/capi/include/blazesym.h +++ b/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; /** diff --git a/capi/src/normalize.rs b/capi/src/normalize.rs index edf6fcac..06406f64 100644 --- a/capi/src/normalize.rs +++ b/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], diff --git a/src/normalize/buildid.rs b/src/normalize/buildid.rs index f55bb9ba..9e6a9b19 100644 --- a/src/normalize/buildid.rs +++ b/src/normalize/buildid.rs @@ -99,7 +99,22 @@ fn read_build_id_impl(parser: &ElfParser) -> Result> { } pub(super) trait BuildIdReader<'src> { - fn read_build_id(&self, path: &Path) -> Result>>; + fn read_build_id(&self, path: &Path) -> Option> { + #[cfg_attr( + feature = "tracing", + crate::log::instrument(err, skip_all, fields(path = ?path)) + )] + fn read_build_id<'src, B>(slf: &B, path: &Path) -> Result>> + 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>>; } @@ -107,8 +122,7 @@ 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>> { + fn read_build_id_fallible(&self, path: &Path) -> Result>> { 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>> { + fn read_build_id_fallible(&self, path: &Path) -> Result>> { 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>> { + fn read_build_id_fallible(&self, _path: &Path) -> Result>> { Ok(None) } } diff --git a/src/normalize/meta.rs b/src/normalize/meta.rs index f6183bdc..1e196e33 100644 --- a/src/normalize/meta.rs +++ b/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>, /// The struct is non-exhaustive and open to extension. #[doc(hidden)] diff --git a/src/normalize/normalizer.rs b/src/normalize/normalizer.rs index a614da0e..94f816f4 100644 --- a/src/normalize/normalizer.rs +++ b/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. diff --git a/src/normalize/user.rs b/src/normalize/user.rs index 50a2b817..d3f59ead 100644 --- a/src/normalize/user.rs +++ b/src/normalize/user.rs @@ -29,7 +29,7 @@ fn make_elf_meta<'src>( ) -> Result> { 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>> { + 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] + ); + } }