feat: Add Unsupported variant to CacheError and ObjectFileStatus (

#1541) This adds a new `CacheError`/`ObjectFileStatus` variant for cases where a debug file is OK, but can't be used for the purpose for which it was requested. Currently the only such case is symbolicating .NET events with portable PDB files (see getsentry/symbolic#871). In addition, this also makes Symbolicator request both portable PDB and Windows PDB files for symbolicating .NET events. The reason for this is that although Windows PDB files can never be used for that purpose, at least this way we get a concrete error status we can report to the user. If we don't do that, we get cases like #1539.
getsentry · Nov 11, 2024 · 2474a74 · 2474a74
1 parent 4535fac
commit 2474a74
Show file tree

Hide file tree

Showing 12 changed files with 160 additions and 7 deletions.
diff --git a/crates/symbolicator-js/src/interface.rs b/crates/symbolicator-js/src/interface.rs
@@ -142,6 +142,7 @@ impl From<CacheError> for JsScrapingResult {
             ),
             CacheError::DownloadError(details) => (JsScrapingFailureReason::DownloadError, details),
             CacheError::Malformed(details) => (JsScrapingFailureReason::Other, details),
+            CacheError::Unsupported(details) => (JsScrapingFailureReason::Other, details),
             CacheError::InternalError => (JsScrapingFailureReason::Other, String::new()),
         };
 

diff --git a/crates/symbolicator-native/src/caches/ppdb_caches.rs b/crates/symbolicator-native/src/caches/ppdb_caches.rs
@@ -58,7 +58,7 @@ impl PortablePdbCacheActor {
         let found_object = self
             .objects
             .find(FindObject {
-                filetypes: &[FileType::PortablePdb],
+                filetypes: &[FileType::PortablePdb, FileType::Pdb],
                 identifier: request.identifier,
                 sources: request.sources,
                 scope: request.scope,
@@ -124,8 +124,12 @@ fn write_ppdb_cache(file: &mut File, object_handle: &ObjectHandle) -> CacheEntry
 
     let ppdb_obj = match object_handle.object() {
         Object::PortablePdb(ppdb_obj) => ppdb_obj,
-        // FIXME(swatinem): instead of panic, we should return an internal error?
-        _ => panic!("object handle does not contain a valid portable pdb object"),
+        _ => {
+            tracing::warn!("Trying to symbolicate a .NET event with a non-PPDB object file");
+            return Err(CacheError::Unsupported(
+                "Only portable PDB files can be used for .NET symbolication".to_owned(),
+            ));
+        }
     };
 
     tracing::debug!("Converting ppdb cache for {}", object_handle.cache_key);

diff --git a/crates/symbolicator-native/src/symbolication/module_lookup.rs b/crates/symbolicator-native/src/symbolication/module_lookup.rs
@@ -47,6 +47,7 @@ pub fn object_file_status_from_cache_entry<T>(cache_entry: &CacheEntry<T>) -> Ob
         }
         Err(CacheError::Timeout(_)) => ObjectFileStatus::Timeout,
         Err(CacheError::Malformed(_)) => ObjectFileStatus::Malformed,
+        Err(CacheError::Unsupported(_)) => ObjectFileStatus::Unsupported,
         Err(CacheError::InternalError) => ObjectFileStatus::Other,
     }
 }

diff --git a/...tive/tests/integration/snapshots/integration__symbolication__dotnet_embedded_sources.snap b/...tive/tests/integration/snapshots/integration__symbolication__dotnet_embedded_sources.snap
@@ -1,5 +1,5 @@
 ---
-source: crates/symbolicator-service/tests/integration/symbolication.rs
+source: crates/symbolicator-native/tests/integration/symbolication.rs
 expression: response.unwrap()
 ---
 stacktraces:
@@ -37,6 +37,10 @@ modules:
         location: "http://localhost:<port>/symbols/portable-embedded.pdb/B6919861510C48879994943F64F70C37870b9ef9/portable-embedded.src.zip"
         download:
           status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/portable-embedded.pdb/B6919861510C48879994943F64F70C37ffffffff/portable-embedded.pd_"
+        download:
+          status: notfound
       - source: local
         location: "http://localhost:<port>/symbols/portable-embedded.pdb/B6919861510C48879994943F64F70C37ffffffff/portable-embedded.pdb"
         download:
@@ -52,4 +56,3 @@ modules:
         location: "http://localhost:<port>/symbols/portable-embedded.pdb/B6919861510C48879994943F64F70C37ffffffff/portable-embedded.src.zip"
         download:
           status: notfound
-
diff --git a/...or-native/tests/integration/snapshots/integration__symbolication__dotnet_integration.snap b/...or-native/tests/integration/snapshots/integration__symbolication__dotnet_integration.snap
@@ -1,5 +1,5 @@
 ---
-source: crates/symbolicator-service/tests/integration/symbolication.rs
+source: crates/symbolicator-native/tests/integration/symbolication.rs
 expression: response.unwrap()
 ---
 stacktraces:
@@ -66,6 +66,10 @@ modules:
         location: "http://localhost:<port>/symbols/integration.pdb/0C1033F78632492E91C6C314B72E1920e60b819d/integration.src.zip"
         download:
           status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/integration.pdb/0C1033F78632492E91C6C314B72E1920ffffffff/integration.pd_"
+        download:
+          status: notfound
       - source: local
         location: "http://localhost:<port>/symbols/integration.pdb/0C1033F78632492E91C6C314B72E1920ffffffff/integration.pdb"
         download:
@@ -81,4 +85,3 @@ modules:
         location: "http://localhost:<port>/symbols/integration.pdb/0C1033F78632492E91C6C314B72E1920ffffffff/integration.src.zip"
         download:
           status: notfound
-
diff --git a/...ive/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap b/...ive/tests/integration/snapshots/integration__symbolication__dotnet_only_source_links.snap
@@ -38,6 +38,10 @@ modules:
         location: "http://localhost:<port>/symbols/source-links.pdb/0C380A12822140698565BEE6B3AC196Ea596286e/source-links.src.zip"
         download:
           status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/source-links.pdb/0C380A12822140698565BEE6B3AC196Effffffff/source-links.pd_"
+        download:
+          status: notfound
       - source: local
         location: "http://localhost:<port>/symbols/source-links.pdb/0C380A12822140698565BEE6B3AC196Effffffff/source-links.pdb"
         download:

diff --git a/...r-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap b/...r-native/tests/integration/snapshots/integration__symbolication__dotnet_source_links.snap
@@ -44,6 +44,10 @@ modules:
         location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55a09672e1/source-links.src.zip"
         download:
           status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.pd_"
+        download:
+          status: notfound
       - source: local
         location: "http://localhost:<port>/symbols/source-links.pdb/37E9E8A61A8E404EB93C6902E277FF55ffffffff/source-links.pdb"
         download:

diff --git a/...or-native/tests/integration/snapshots/integration__symbolication__dotnet_windows_pdb.snap b/...or-native/tests/integration/snapshots/integration__symbolication__dotnet_windows_pdb.snap
@@ -0,0 +1,68 @@
+---
+source: crates/symbolicator-native/tests/integration/symbolication.rs
+expression: response.unwrap()
+---
+stacktraces:
+  - frames:
+      - status: missing
+        original_index: 0
+        addr_mode: "rel:0"
+        instruction_addr: "0xa"
+        function_id: "0x6"
+      - status: missing
+        original_index: 1
+        addr_mode: "rel:0"
+        instruction_addr: "0x6"
+        function_id: "0x5"
+      - status: missing
+        original_index: 2
+        addr_mode: "rel:0"
+        instruction_addr: "0x0"
+        function_id: "0x3"
+      - status: missing
+        original_index: 3
+        addr_mode: "rel:0"
+        instruction_addr: "0x0"
+        function_id: "0x2"
+      - status: missing
+        original_index: 4
+        addr_mode: "rel:0"
+        instruction_addr: "0x2d"
+        function_id: "0x1"
+modules:
+  - debug_status: unsupported
+    features:
+      has_debug_info: true
+      has_unwind_info: true
+      has_symbols: true
+      has_sources: false
+    arch: unknown
+    type: pe_dotnet
+    debug_id: 3249d99d-0c40-4931-8610-f4e4fb0b6936
+    debug_file: crash.pdb
+    image_addr: "0x0"
+    candidates:
+      - source: local
+        location: "http://localhost:<port>/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B69360/crash.src.zip"
+        download:
+          status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B6936ffffffff/crash.pd_"
+        download:
+          status: notfound
+      - source: local
+        location: "http://localhost:<port>/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B6936ffffffff/crash.pdb"
+        download:
+          status: ok
+          features:
+            has_debug_info: true
+            has_unwind_info: true
+            has_symbols: true
+            has_sources: false
+        debug:
+          status: error
+          details: "unsupported: Only portable PDB files can be used for .NET symbolication"
+      - source: local
+        location: "http://localhost:<port>/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B6936ffffffff/crash.src.zip"
+        download:
+          status: notfound
diff --git a/crates/symbolicator-native/tests/integration/symbolication.rs b/crates/symbolicator-native/tests/integration/symbolication.rs
@@ -156,6 +156,48 @@ async fn test_dotnet_integration() {
     assert_snapshot!(response.unwrap());
 }
 
+#[tokio::test]
+async fn test_dotnet_windows_pdb() {
+    let (symbolication, _cache_dir) = setup_service(|_| ());
+    let (_srv, source) = symbol_server();
+
+    // try to symbolicate a `PE_DOTNET` event with a Windows PDB file, this should fail
+    let request = make_symbolication_request(
+        vec![source],
+        r#"[{
+          "type":"pe_dotnet",
+          "debug_file":"crash.pdb",
+          "debug_id":"3249d99d0c4049318610f4e4fb0b6936"
+        }]"#,
+        r#"[{
+          "frames":[{
+            "instruction_addr": 10,
+            "function_id": 6,
+            "addr_mode":"rel:0"
+          }, {
+            "instruction_addr": 6,
+            "function_id": 5,
+            "addr_mode": "rel:0"
+          }, {
+            "instruction_addr": 0,
+            "function_id": 3,
+            "addr_mode": "rel:0"
+          }, {
+            "instruction_addr": 0,
+            "function_id": 2,
+            "addr_mode": "rel:0"
+          }, {
+            "instruction_addr": 45,
+            "function_id": 1,
+            "addr_mode": "rel:0"
+          }]
+        }]"#,
+    );
+    let response = symbolication.symbolicate(request).await;
+
+    assert_snapshot!(response.unwrap());
+}
+
 #[tokio::test]
 async fn test_dotnet_embedded_sources() {
     let (symbolication, _cache_dir) = setup_service(|_| ());

diff --git a/crates/symbolicator-service/src/caching/cache_error.rs b/crates/symbolicator-service/src/caching/cache_error.rs
@@ -38,6 +38,14 @@ pub enum CacheError {
     /// during symcache conversion
     #[error("malformed: {0}")]
     Malformed(String),
+    /// The object is of a type that cannot be used for the symbolication task it was
+    /// requested for.
+    ///
+    /// This is currently only used when we try to symbolicate a .NET event with a Windows
+    /// PDB file. A tracking issue in `symbolic` for supporting this case is
+    /// [here](https://github.com/getsentry/symbolic/issues/871).
+    #[error("unsupported: {0}")]
+    Unsupported(String),
     /// An unexpected error in symbolicator itself.
     ///
     /// This variant is not intended to be persisted to or read from caches.
@@ -64,6 +72,7 @@ impl CacheError {
     pub(super) const PERMISSION_DENIED_MARKER: &'static [u8] = b"permissiondenied";
     pub(super) const TIMEOUT_MARKER: &'static [u8] = b"timeout";
     pub(super) const DOWNLOAD_ERROR_MARKER: &'static [u8] = b"downloaderror";
+    pub(super) const UNSUPPORTED_MARKER: &'static [u8] = b"unsupported";
 
     /// Writes error markers and details to a file.
     ///
@@ -99,6 +108,10 @@ impl CacheError {
                 file.write_all(Self::DOWNLOAD_ERROR_MARKER).await?;
                 file.write_all(details.as_bytes()).await?;
             }
+            CacheError::Unsupported(details) => {
+                file.write_all(Self::UNSUPPORTED_MARKER).await?;
+                file.write_all(details.as_bytes()).await?;
+            }
             CacheError::InternalError => {
                 unreachable!("this was already handled above");
             }
@@ -134,6 +147,9 @@ impl CacheError {
         } else if let Some(raw_message) = bytes.strip_prefix(Self::MALFORMED_MARKER) {
             let err_msg = utf8_message(raw_message);
             Some(Self::Malformed(err_msg.into_owned()))
+        } else if let Some(raw_message) = bytes.strip_prefix(Self::UNSUPPORTED_MARKER) {
+            let err_msg = utf8_message(raw_message);
+            Some(Self::Unsupported(err_msg.into_owned()))
         } else if bytes.is_empty() {
             Some(Self::NotFound)
         } else {

diff --git a/crates/symbolicator-service/src/types.rs b/crates/symbolicator-service/src/types.rs
@@ -147,6 +147,12 @@ pub enum ObjectFileStatus {
     FetchingFailed,
     /// Downloading or processing the file took too long.
     Timeout,
+    /// The file could not be used for the purpose for which it was requested.
+    ///
+    /// This is currently only used when we try to symbolicate a .NET event with a Windows
+    /// PDB file. A tracking issue in `symbolic` for supporting this case is
+    /// [here](https://github.com/getsentry/symbolic/issues/871).
+    Unsupported,
     /// An internal error while handling this image.
     Other,
 }
@@ -161,6 +167,7 @@ impl ObjectFileStatus {
             ObjectFileStatus::Malformed => "malformed",
             ObjectFileStatus::FetchingFailed => "fetching_failed",
             ObjectFileStatus::Timeout => "timeout",
+            ObjectFileStatus::Unsupported => "unsupported",
             ObjectFileStatus::Other => "other",
         }
     }

diff --git a/tests/fixtures/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B6936ffffffff/crash.pdb b/tests/fixtures/symbols/crash.pdb/3249D99D0C4049318610F4E4FB0B6936ffffffff/crash.pdb