Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(symsorter): Don't emit files with empty debug identifiers #469

Merged
merged 2 commits into from
Jun 17, 2021
Merged

fix(symsorter): Don't emit files with empty debug identifiers #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3270271
fix(symsorter): Don't emit files with empty debug identifiers
relaxolotl Jun 16, 2021
535da7c
meta: Changelog
jan-auer Jun 17, 2021
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- Bump symbolic to support versioning of CFI Caches. ([#467](https://github.com/getsentry/symbolicator/pull/467))

### Tools

- `symsorter` no longer emits files with empty debug identifiers. ([#469](https://github.com/getsentry/symbolicator/pull/469))

## 0.3.4

### Features
Expand Down
7 changes: 3 additions & 4 deletions crates/symsorter/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,7 @@ fn process_file(

for obj in archive.objects() {
let obj = maybe_ignore_error!(obj.map_err(|e| anyhow!(e)));
let new_filename = root.join(maybe_ignore_error!(
get_target_filename(&obj).ok_or_else(|| anyhow!("unsupported file"))
));
let new_filename = root.join(maybe_ignore_error!(get_target_filename(&obj)));

fs::create_dir_all(new_filename.parent().unwrap())?;

Expand Down Expand Up @@ -160,7 +158,8 @@ fn process_file(
} else {
io::copy(&mut obj.data(), &mut out)?;
}
rv.push((get_unified_id(&obj), obj.kind()));
let unified_id = maybe_ignore_error!(get_unified_id(&obj));
rv.push((unified_id, obj.kind()));
}

Ok(rv)
Expand Down
31 changes: 15 additions & 16 deletions crates/symsorter/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::io::Cursor;
use std::path::{Path, PathBuf};

use anyhow::{anyhow, Result};
use anyhow::{anyhow, bail, Result};
use lazy_static::lazy_static;
use regex::Regex;
use symbolic::common::ByteView;
Expand All @@ -26,31 +26,30 @@ pub fn is_bundle_id(input: &str) -> bool {
}

/// Gets the unified ID from an object.
pub fn get_unified_id(obj: &Object) -> String {
pub fn get_unified_id(obj: &Object) -> Result<String> {
if obj.file_format() == FileFormat::Pe || obj.code_id().is_none() {
obj.debug_id().breakpad().to_string().to_lowercase()
let debug_id = obj.debug_id();
if debug_id.is_nil() {
Err(anyhow!("failed to generate debug identifier"))
} else {
Ok(debug_id.breakpad().to_string().to_lowercase())
}
} else {
obj.code_id().as_ref().unwrap().as_str().to_string()
Ok(obj.code_id().as_ref().unwrap().as_str().to_string())
Copy link
Member

Choose a reason for hiding this comment

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

Could you check that this identifier is non-empty, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this also tripped me up as well when I was working on this: L30 checks this (|| obj.code_id().is_none()) and enters the debug ID block if .code_id() is empty.

I was thinking of ways to make it more obvious that this else block runs with a non-empty code_id, but anything that came to mind felt like an overcomplication. For example, a match block was one of my ideas:

match (obj.file_format(), obj.code_id()) {
    (Pe, _) 
    | (_, None) => use debug_id,
    (_, _) => use code
}

Thoughts, opinions?

Copy link
Member

@jan-auer jan-auer Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Heh, you know our code better than we do. And in fact, code_id can also never be empty since it always returns None instead. This is good to go then 👍

}
}

/// Returns the intended target filename for an object.
pub fn get_target_filename(obj: &Object) -> Option<PathBuf> {
let id = get_unified_id(obj);
pub fn get_target_filename(obj: &Object) -> Result<PathBuf> {
let id = get_unified_id(obj)?;
// match the unified format here.
let suffix = match obj.kind() {
ObjectKind::Debug => "debuginfo",
ObjectKind::Sources => {
if obj.file_format() == FileFormat::SourceBundle {
"sourcebundle"
} else {
return None;
}
}
ObjectKind::Sources if obj.file_format() == FileFormat::SourceBundle => "sourcebundle",
ObjectKind::Relocatable | ObjectKind::Library | ObjectKind::Executable => "executable",
_ => return None,
_ => bail!("unsupported file"),
};
Some(format!("{}/{}/{}", &id[..2], &id[2..], suffix).into())
Ok(format!("{}/{}/{}", &id[..2], &id[2..], suffix).into())
}

/// Creates a source bundle from a path.
Expand All @@ -59,7 +58,7 @@ pub fn create_source_bundle(path: &Path, unified_id: &str) -> Result<Option<Byte
let archive = Archive::parse(&bv).map_err(|e| anyhow!(e))?;
for obj in archive.objects() {
let obj = obj.map_err(|e| anyhow!(e))?;
if get_unified_id(&obj) == unified_id {
if matches!(get_unified_id(&obj), Ok(generated_id) if unified_id == generated_id) {
let mut out = Vec::<u8>::new();
let writer =
SourceBundleWriter::start(Cursor::new(&mut out)).map_err(|e| anyhow!(e))?;
Expand Down