feat: extract and upload embedded Portable PDB from PE (#1463)

Cargo.toml

@@ -61,7 +61,7 @@ serde = { version = "1.0.152", features = ["derive"] }
 serde_json = "1.0.91"
 sha1_smol = { version = "1.0.0", features = ["serde"] }
 sourcemap = { version = "6.2.1", features = ["ram_bundle"] }
-symbolic = { version = "11.0.0", features = ["debuginfo-serde", "il2cpp"] }
+symbolic = { version = "11.1.0", features = ["debuginfo-serde", "il2cpp"] }
 thiserror = "1.0.38"
 url = "2.3.1"
 username = "0.2.0"

src/utils/dif.rs

@@ -335,11 +335,30 @@ impl<'a> DifFile<'a> {
         match self {
             DifFile::Archive(archive) => {
                 let mut features = ObjectDifFeatures::none();
-                for object in archive.get().objects().filter_map(Result::ok) {
+
+                let mut add_object_features = |object: &Object| {
                     features.symtab = features.symtab || object.has_symbols();
                     features.debug = features.debug || object.has_debug_info();
                     features.unwind = features.unwind || object.has_unwind_info();
                     features.sources = features.sources || object.has_sources();
+                };
+
+                for object in archive.get().objects().filter_map(Result::ok) {
+                    add_object_features(&object);
+
+                    // Combine features with an embedded Portable PDB, if any to show up correctly in `dif check` cmd.
+                    // Note: this is intentionally different than `DifUpload.valid_features()` because we don't want to
+                    // upload the PE file separately, unless it has features we need. The PPDB is extracted instead.
+                    if let Ok(Some(Object::Pe(pe))) = archive.get().object_by_index(0) {
+                        if let Ok(Some(ppdb_data)) = pe.embedded_ppdb() {
+                            let mut buf = Vec::new();
+                            if ppdb_data.decompress_to(&mut buf).is_ok() {
+                                if let Ok(ppdb) = Object::parse(&buf) {
+                                    add_object_features(&ppdb);
+                                }
+                            }
+                        }
+                    }
                 }
                 features
             }

src/utils/dif_upload.rs

@@ -24,6 +24,7 @@ use log::{debug, info, warn};
 use sha1_smol::Digest;
 use symbolic::common::{AsSelf, ByteView, DebugId, SelfCell, Uuid};
 use symbolic::debuginfo::macho::{BcSymbolMap, UuidMapping};
+use symbolic::debuginfo::pe::PeObject;
 use symbolic::debuginfo::sourcebundle::SourceBundleWriter;
 use symbolic::debuginfo::{Archive, FileEntry, FileFormat, Object};
 use symbolic::il2cpp::ObjectLineMapping;
@@ -114,7 +115,7 @@ struct DifMatch<'data> {
 }
 
 impl<'data> DifMatch<'data> {
-    fn from_temp_object<S>(temp_file: TempFile, name: S) -> Result<Self>
+    fn from_temp_object<S>(temp_file: TempFile, name: S, debug_id: Option<DebugId>) -> Result<Self>
     where
         S: Into<String>,
     {
@@ -123,14 +124,11 @@ impl<'data> DifMatch<'data> {
             Object::parse(unsafe { &*b }).map(|object| ParsedDif::Object(Box::new(object)))
         })?;
 
-        // Even though we could supply the debug_id here from the object we do not, the
-        // server will do the same anyway and we actually have control over the version of
-        // the code running there so can fix bugs more reliably.
         Ok(DifMatch {
             _backing: Some(DifBacking::Temp(temp_file)),
             dif,
             name: name.into(),
-            debug_id: None,
+            debug_id,
             attachments: None,
         })
     }
@@ -196,8 +194,11 @@ impl<'data> DifMatch<'data> {
         P: AsRef<Path>,
         S: Into<String>,
     {
+        // Even though we could supply the debug_id here from the object we do not, the
+        // server will do the same anyway and we actually have control over the version of
+        // the code running there so can fix bugs more reliably.
         let temp_file = TempFile::take(path)?;
-        Self::from_temp_object(temp_file, name)
+        Self::from_temp_object(temp_file, name, None)
     }
 
     /// Returns the parsed [`Object`] of this DIF.
@@ -729,16 +730,13 @@ fn search_difs(options: &DifUpload) -> Result<Vec<DifMatch<'static>>> {
         }
     );
 
-    let count_with_sources = match options.include_sources {
-        true => collected
-            .iter()
-            .filter(|dif| match dif.object() {
-                Some(object) => object.has_sources(),
-                None => false,
-            })
-            .count(),
-        false => 0,
-    };
+    let count_with_sources = collected
+        .iter()
+        .filter(|dif| match dif.object() {
+            Some(object) => object.has_sources(),
+            None => false,
+        })
+        .count();
 
     match count_with_sources {
         0 => println!(),
@@ -853,7 +851,7 @@ fn collect_object_dif<'a>(
 
     // Each `FatObject` might contain multiple matching objects, each of
     // which needs to retain a reference to the original fat file. We
-    // create a shared instance here and clone it into `DifMatche`s
+    // create a shared instance here and clone it into `DifMatch`es
     // below.
     for object in archive.objects() {
         // Silently skip all objects that we cannot process. This can
@@ -872,6 +870,15 @@ fn collect_object_dif<'a>(
             continue;
         }
 
+        // If this is a PE file with an embedded Portable PDB, we extract and process the PPDB separately.
+        if let Object::Pe(pe) = &object {
+            if let Ok(Some(ppdb_dif)) = extract_embedded_ppdb(pe, name.as_str()) {
+                if options.validate_dif(&ppdb_dif) {
+                    collected.push(ppdb_dif);
+                }
+            }
+        };
+
         // Store a mapping of "age" values for all encountered PE files,
         // regardless of whether they will be uploaded. This is used later
         // to fix up PDB files.
@@ -1077,6 +1084,25 @@ fn process_symbol_maps<'a>(
     Ok(without_hidden)
 }
 
+/// Checks whether the `PeObject` contains an embedded Portable PDB and extracts it as a separate  `DifMatch`.
+fn extract_embedded_ppdb<'a>(pe: &PeObject, pe_name: &str) -> Result<Option<DifMatch<'a>>> {
+    if let Some(embedded_ppdb) = pe.embedded_ppdb()? {
+        let temp_file = TempFile::create()?;
+        temp_file
+            .open()
+            .map(|f| embedded_ppdb.decompress_to(BufWriter::new(f)))??;
+
+        let dif = DifMatch::from_temp_object(
+            temp_file,
+            Path::new(pe_name).with_extension("pdb").to_string_lossy(),
+            Some(pe.debug_id()),
+        )?;
+        Ok(Some(dif))
+    } else {
+        Ok(None)
+    }
+}
+
 /// Default filter function to skip over bad sources we do not want to include.
 pub fn filter_bad_sources(entry: &FileEntry) -> bool {
     let max_size = Config::current().get_max_dif_item_size();
@@ -1151,9 +1177,7 @@ fn create_source_bundles<'a>(
             continue;
         }
 
-        let mut source_bundle = DifMatch::from_temp_object(temp_file, name)?;
-        source_bundle.debug_id = dif.debug_id;
-        source_bundles.push(source_bundle);
+        source_bundles.push(DifMatch::from_temp_object(temp_file, name, dif.debug_id)?);
     }
 
     let len = source_bundles.len();

tests/integration/_cases/debug_files/debug_files-check-dll-embedded-ppdb-with-sources.trycmd

@@ -0,0 +1,14 @@
+```
+$ SENTRY_ALLOW_FAILURE=1 sentry-cli debug-files check tests/integration/_fixtures/Sentry.Samples.Console.Basic-embedded-ppdb-with-sources.dll
+? success
+Debug Info File Check
+  Type: pe executable
+  Contained debug identifiers:
+    > Debug ID: 623535c7-c0ea-4dee-b99b-4669e99a7ecc-a878d1fa
+      Code ID:  d1dde1d5a000
+      Arch:     x86
+  Contained debug information:
+    > debug, sources
+  Usable: yes
+
+```

tests/integration/_cases/debug_files/debug_files-upload-dll-embedded-ppdb-with-sources.trycmd

@@ -0,0 +1,8 @@
+```
+$ sentry-cli debug-files upload tests/integration/_fixtures/Sentry.Samples.Console.Basic-embedded-ppdb-with-sources.dll
+? success
+> Found 1 debug information file (1 with embedded sources)
+> Prepared debug information file for upload
+> Nothing to upload, all files are on the server
+
+```

tests/integration/debug_files/check.rs

@@ -28,3 +28,8 @@ fn command_debug_files_check_no_file_allow_failure_env() {
     #[cfg(windows)]
     register_test("debug_files/debug_files-check-no-file-allow-failure-env-windows.trycmd");
 }
+
+#[test]
+fn command_debug_files_check_dll_embedded_ppdb_with_sources() {
+    register_test("debug_files/debug_files-check-dll-embedded-ppdb-with-sources.trycmd");
+}

tests/integration/debug_files/upload.rs

@@ -99,6 +99,38 @@ fn command_debug_files_upload_pdb_embedded_sources() {
     register_test("debug_files/debug_files-upload-pdb-embedded-sources.trycmd");
 }
 
+#[test]
+fn command_debug_files_upload_dll_embedded_ppdb_with_sources() {
+    let _chunk_upload = mock_endpoint(
+        EndpointOptions::new("GET", "/api/0/organizations/wat-org/chunk-upload/", 200)
+            .with_response_file("debug_files/get-chunk-upload.json"),
+    );
+    let _assemble = mock_endpoint(
+        EndpointOptions::new(
+            "POST",
+            "/api/0/projects/wat-org/wat-project/files/difs/assemble/",
+            200,
+        )
+        .with_response_body(
+            r#"{
+                "fc1c9e58a65bd4eaf973bbb7e7a7cc01bfdaf15e": {
+                    "state": "ok",
+                    "missingChunks": []
+                }
+            }"#,
+        ),
+    );
+    let _reprocessing = mock_endpoint(
+        EndpointOptions::new(
+            "POST",
+            "/api/0/projects/wat-org/wat-project/reprocessing/",
+            200,
+        )
+        .with_response_body("[]"),
+    );
+    register_test("debug_files/debug_files-upload-dll-embedded-ppdb-with-sources.trycmd");
+}
+
 #[test]
 fn command_debug_files_upload_no_upload() {
     let _chunk_upload = mock_endpoint(