Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ fallible-iterator = "0.3.0"
flate2 = { version = "1.1.8", default-features = false, features = [
"rust_backend",
] }
gimli = { version = "0.32.3", default-features = false, features = [
gimli = { version = "0.33.0", default-features = false, features = [
"read",
"std",
"fallible-iterator",
] }
goblin = { version = "0.8.0", default-features = false }
goblin = { version = "0.10.5", default-features = false }
indexmap = "2.0.0"
insta = { version = "1.28.0", features = ["yaml"] }
itertools = "0.14.0"
Expand All @@ -49,7 +49,7 @@ proptest = "1.6.0"
regex = "1.7.1"
rustc-demangle = "0.1.21"
# keep this in sync with whatever version `goblin` uses
scroll = "0.12.0"
scroll = "0.13.0"
serde = { version = "1.0.171", features = ["derive"] }
serde_json = "1.0.102"
similar-asserts = "1.4.2"
Expand All @@ -62,7 +62,7 @@ time = { version = "0.3.20", features = ["formatting"] }
tokio = "1.36.0"
tracing = "0.1.34"
tracing-subscriber = "0.3.11"
uuid = "1.3.0"
Comment thread
loewenheim marked this conversation as resolved.
uuid = "1.23.0"
walkdir = "2.3.1"
wasmparser = "0.243.0"
watto = { version = "0.2.0", features = ["writer", "strings"] }
Expand Down
43 changes: 22 additions & 21 deletions symbolic-debuginfo/src/dwarf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ type RangeLists<'a> = gimli::read::RangeLists<Slice<'a>>;
type Unit<'a> = gimli::read::Unit<Slice<'a>>;
type DwarfInner<'a> = gimli::read::Dwarf<Slice<'a>>;

type Die<'d, 'u> = gimli::read::DebuggingInformationEntry<'u, 'u, Slice<'d>, usize>;
type Die<'d, 'u> = gimli::read::DebuggingInformationEntry<Slice<'d>, usize>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type Die<'d, 'u> = gimli::read::DebuggingInformationEntry<Slice<'d>, usize>;
type Die<'d> = gimli::read::DebuggingInformationEntry<Slice<'d>, usize>;

Seems like 'u is never used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that you're allowed to have unused lifetime parameters in type aliases, but not type definitions (and unused type parameters aren't allowed in either).

type Attribute<'a> = gimli::read::Attribute<Slice<'a>>;
type UnitOffset = gimli::read::UnitOffset<usize>;
type DebugInfoOffset = gimli::DebugInfoOffset<usize>;
type EntriesRaw<'d, 'u> = gimli::EntriesRaw<'u, 'u, Slice<'d>>;
type EntriesRaw<'d, 'u> = gimli::EntriesRaw<'u, Slice<'d>>;

type UnitHeader<'a> = gimli::read::UnitHeader<Slice<'a>>;
type IncompleteLineNumberProgram<'a> = gimli::read::IncompleteLineProgram<Slice<'a>>;
Expand Down Expand Up @@ -416,11 +416,10 @@ impl<'d> UnitRef<'d, '_> {
/// Returns the source language declared in the root DIE of this compilation unit.
fn language(&self) -> Result<Option<Language>, DwarfError> {
let mut entries = self.unit.entries();
let Some((_, root_entry)) = entries.next_dfs()? else {
let Some(root_entry) = entries.next_dfs()? else {
return Ok(None);
};
let Some(AttributeValue::Language(lang)) =
root_entry.attr_value(constants::DW_AT_language)?
let Some(AttributeValue::Language(lang)) = root_entry.attr_value(constants::DW_AT_language)
else {
return Ok(None);
};
Expand All @@ -442,8 +441,8 @@ impl<'d> UnitRef<'d, '_> {
if depth == 0 {
return Ok(None);
}
if let Ok(Some(attr)) = entry.attr(constants::DW_AT_abstract_origin) {
return self.resolve_reference(attr, |ref_unit, ref_entry| {
if let Some(attr) = entry.attr(constants::DW_AT_abstract_origin) {
return self.resolve_reference(*attr, |ref_unit, ref_entry| {
// Recurse first to follow deeper chains.
if let Some(lang) = ref_unit.resolve_entry_language(ref_entry, depth - 1)? {
return Ok(Some(lang));
Expand All @@ -467,11 +466,10 @@ impl<'d> UnitRef<'d, '_> {
bcsymbolmap: Option<&'d BcSymbolMap<'d>>,
prior_offset: Option<UnitOffset>,
) -> Result<Option<Name<'d>>, DwarfError> {
let mut attrs = entry.attrs();
let mut fallback_name = None;
let mut reference_target = None;

while let Some(attr) = attrs.next()? {
for attr in entry.attrs() {
match attr.name() {
// Prioritize these. If we get them, take them.
constants::DW_AT_linkage_name | constants::DW_AT_MIPS_linkage_name => {
Expand All @@ -498,7 +496,7 @@ impl<'d> UnitRef<'d, '_> {
}

if let Some(attr) = reference_target {
return self.resolve_reference(attr, |ref_unit, ref_entry| {
return self.resolve_reference(*attr, |ref_unit, ref_entry| {
// Self-references may have a layer of indirection. Avoid infinite recursion
// in this scenario.
if let Some(prior) = prior_offset {
Expand Down Expand Up @@ -544,7 +542,7 @@ impl<'d, 'a> DwarfUnit<'d, 'a> {
let inner = UnitRef { info, unit };
let mut entries = unit.entries();
let entry = match entries.next_dfs()? {
Some((_, entry)) => entry,
Some(entry) => entry,
None => return Err(gimli::read::Error::MissingUnitDie.into()),
};

Expand All @@ -554,12 +552,12 @@ impl<'d, 'a> DwarfUnit<'d, 'a> {
// range information has not been written yet and all units look like this.
if info.kind != ObjectKind::Relocatable
&& unit.low_pc == 0
&& entry.attr(constants::DW_AT_ranges)?.is_none()
&& entry.attr(constants::DW_AT_ranges).is_none()
{
return Ok(None);
}

let language = match entry.attr_value(constants::DW_AT_language)? {
let language = match entry.attr_value(constants::DW_AT_language) {
Some(AttributeValue::Language(lang)) => language_from_dwarf(lang),
_ => Language::Unknown,
};
Expand All @@ -573,7 +571,7 @@ impl<'d, 'a> DwarfUnit<'d, 'a> {
// reference into the debug_str section. We use `string_value`
// to resolve it correctly in either case.
let producer = entry
.attr_value(constants::DW_AT_producer)?
.attr_value(constants::DW_AT_producer)
.and_then(|av| av.string_value(&info.inner.debug_str));

// Trust the symbol table more to contain accurate mangled names. However, since Dart's name
Expand Down Expand Up @@ -626,23 +624,23 @@ impl<'d, 'a> DwarfUnit<'d, 'a> {
AttributeValue::DebugAddrIndex(index) => {
low_pc = Some(self.inner.info.address(self.inner.unit, index)?)
}
_ => return Err(GimliError::UnsupportedAttributeForm.into()),
_ => return Err(GimliError::UnsupportedAttributeForm(attr.form()).into()),
},
constants::DW_AT_high_pc => match attr.value() {
AttributeValue::Addr(addr) => high_pc = Some(addr),
AttributeValue::DebugAddrIndex(index) => {
high_pc = Some(self.inner.info.address(self.inner.unit, index)?)
}
AttributeValue::Udata(size) => high_pc_rel = Some(size),
_ => return Err(GimliError::UnsupportedAttributeForm.into()),
_ => return Err(GimliError::UnsupportedAttributeForm(attr.form()).into()),
},
constants::DW_AT_call_line => match attr.value() {
AttributeValue::Udata(line) => call_line = Some(line),
_ => return Err(GimliError::UnsupportedAttributeForm.into()),
_ => return Err(GimliError::UnsupportedAttributeForm(attr.form()).into()),
},
constants::DW_AT_call_file => match attr.value() {
AttributeValue::FileIndex(file) => call_file = Some(file),
_ => return Err(GimliError::UnsupportedAttributeForm.into()),
_ => return Err(GimliError::UnsupportedAttributeForm(attr.form()).into()),
},
constants::DW_AT_ranges
| constants::DW_AT_rnglists_base
Expand All @@ -656,7 +654,7 @@ impl<'d, 'a> DwarfUnit<'d, 'a> {
// are triggered by an inverted range (going high to low).
// See a few more examples of broken ranges here:
// https://github.com/emscripten-core/emscripten/issues/15552
Err(gimli::Error::InvalidAddressRange) => None,
Err(gimli::Error::InvalidCfiSetLoc(_)) => None,
Comment thread
sentry[bot] marked this conversation as resolved.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong error variant caught for broken range lists

High Severity

The error variant was changed from gimli::Error::InvalidAddressRange to gimli::Error::InvalidCfiSetLoc(_), but these are semantically unrelated. InvalidCfiSetLoc relates to Call Frame Information parsing, not address range validation. This catch clause exists to gracefully handle broken WASM debug files from emscripten with inverted ranges (begin > end), as the comment explains. Since range list iteration will never produce an InvalidCfiSetLoc error, this match arm is dead code and those broken files will now trigger unhandled errors instead of being silently skipped.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a90289b. Configure here.

Comment on lines -659 to +657
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 0.32, the InvalidAddressRange error variant is returned only in the evaluate function:
https://github.com/gimli-rs/gimli/blob/71720829ef6f61ea5b0d4e1b9315534d2c8b9264/src/read/cfi.rs#L2325-L2327

However, in 0.33, this variant is removed and InvalidCfiSetLoc is now returned at the same location:
https://github.com/gimli-rs/gimli/blob/8e70a8accfef4472dd09dbcd7cdbfdefe4803c6b/src/read/cfi.rs#L2369-L2371

Err(err) => {
return Err(err.into());
}
Expand Down Expand Up @@ -1175,6 +1173,7 @@ struct DwarfSections<'data> {
debug_info: DwarfSectionData<'data, gimli::read::DebugInfo<Slice<'data>>>,
debug_line: DwarfSectionData<'data, gimli::read::DebugLine<Slice<'data>>>,
debug_line_str: DwarfSectionData<'data, gimli::read::DebugLineStr<Slice<'data>>>,
debug_names: DwarfSectionData<'data, gimli::read::DebugNames<Slice<'data>>>,
debug_str: DwarfSectionData<'data, gimli::read::DebugStr<Slice<'data>>>,
debug_str_offsets: DwarfSectionData<'data, gimli::read::DebugStrOffsets<Slice<'data>>>,
debug_ranges: DwarfSectionData<'data, gimli::read::DebugRanges<Slice<'data>>>,
Expand All @@ -1196,6 +1195,7 @@ impl<'data> DwarfSections<'data> {
debug_info: DwarfSectionData::load(dwarf),
debug_line: DwarfSectionData::load(dwarf),
debug_line_str: DwarfSectionData::load(dwarf),
debug_names: DwarfSectionData::load(dwarf),
debug_str: DwarfSectionData::load(dwarf),
debug_str_offsets: DwarfSectionData::load(dwarf),
debug_ranges: DwarfSectionData::load(dwarf),
Expand Down Expand Up @@ -1239,6 +1239,7 @@ impl<'d> DwarfInfo<'d> {
debug_info: sections.debug_info.to_gimli(),
debug_line: sections.debug_line.to_gimli(),
debug_line_str: sections.debug_line_str.to_gimli(),
debug_names: sections.debug_names.to_gimli(),
debug_str: sections.debug_str.to_gimli(),
debug_str_offsets: sections.debug_str_offsets.to_gimli(),
debug_types: Default::default(),
Expand All @@ -1255,7 +1256,7 @@ impl<'d> DwarfInfo<'d> {
inner.populate_abbreviations_cache(AbbreviationsCacheStrategy::Duplicates);

// Prepare random access to unit headers.
let headers = inner.units().collect::<Vec<_>>()?;
let headers = FallibleIterator::collect::<Vec<_>>(inner.units())?;
let units = headers.iter().map(|_| OnceCell::new()).collect();

Ok(DwarfInfo {
Expand Down Expand Up @@ -1297,7 +1298,7 @@ impl<'d> DwarfInfo<'d> {
&self,
offset: DebugInfoOffset,
) -> Result<(UnitRef<'d, '_>, UnitOffset), DwarfError> {
let section_offset = UnitSectionOffset::DebugInfoOffset(offset);
let section_offset = UnitSectionOffset(offset.0);
let search_result = self
.headers
.binary_search_by_key(&section_offset, UnitHeader::offset);
Expand Down
12 changes: 8 additions & 4 deletions symbolic-debuginfo/src/pe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,11 @@ impl<'data> PeObject<'data> {
debug_data
.codeview_pdb70_debug_info
.as_ref()
.map(|cv_record| (debug_data.image_debug_directory, cv_record))
.map(|cv_record| {
let debug_directory =
debug_data.find_type(goblin::pe::debug::IMAGE_DEBUG_TYPE_CODEVIEW);
(debug_directory, cv_record)
})
})
.and_then(|(debug_directory, cv_record)| {
let guid = &cv_record.signature;
Expand All @@ -136,8 +140,8 @@ impl<'data> PeObject<'data> {
// > Matching PDB ID is stored in the #Pdb stream of the .pdb file.
//
// See https://github.com/dotnet/runtime/blob/main/docs/design/specs/PE-COFF.md#codeview-debug-directory-entry-type-2
let age = if debug_directory.minor_version == 0x504d {
debug_directory.time_date_stamp
let age = if let Some(d) = debug_directory.filter(|d| d.minor_version == 0x504d) {
d.time_date_stamp
} else {
cv_record.age
};
Expand Down Expand Up @@ -191,7 +195,7 @@ impl<'data> PeObject<'data> {
/// load address, so that the caller only has to deal with addresses relative to the actual
/// start of the image.
pub fn load_address(&self) -> u64 {
self.pe.image_base as u64
self.pe.image_base
}

/// Determines whether this object exposes a public symbol table.
Expand Down
Loading