Repository: luser/rust-minidump
Pull request #419 (2022-01-31 21:27:56)

Avoid overflow when getting symbol lines
Fixes: #413 

Gankra: I think this would be a bit cleaner to rewrite as something like 

```
if size > 0 { 
  if let Some(x) = checked_add {
    return (Some ....)
  }
}
(None, l)
 ```
 
 Make it more clear that there's just one success case and everything else is the fail case.

Patch:
diff --git a/breakpad-symbols/src/sym_file/parser.rs b/breakpad-symbols/src/sym_file/parser.rs
index c6f67b8..2f72593 100644
--- a/breakpad-symbols/src/sym_file/parser.rs
+++ b/breakpad-symbols/src/sym_file/parser.rs
@@ -514,13 +514,12 @@ impl SymbolParser {
                         // Line data from PDB files often has a zero-size line entry, so just
                         // filter those out.
                         if l.size > 0 {
-                            (
-                                Some(Range::new(l.address, l.address + l.size as u64 - 1)),
-                                l,
-                            )
-                        } else {
-                            (None, l)
+                            if let Some(end) = l.address.checked_add(l.size as u64 - 1) {
+                                return (Some(Range::new(l.address, end)), l);
+                            }
                         }
+
+                        (None, l)
                     })
                     .into_rangemap_safe();
 
@@ -1282,3 +1281,12 @@ STACK WIN 4 8d93e 4 4 0 0 10 0 0 1 prog string
         );
     }
 }
+
+#[test]
+fn address_size_overflow() {
+    let bytes = b"FUNC 1 2 3 x\nffffffffffffffff 2 0 0\n";
+    let sym = parse_symbol_bytes(bytes.as_slice()).unwrap();
+    let fun = sym.functions.get(1).unwrap();
+    assert!(fun.lines.is_empty());
+    assert!(fun.name == "x");
+}
diff --git a/minidump-processor/src/process_state.rs b/minidump-processor/src/process_state.rs
index aed7206..d8bb12c 100644
--- a/minidump-processor/src/process_state.rs
+++ b/minidump-processor/src/process_state.rs
@@ -131,6 +131,8 @@ pub struct CallStack {
     pub frames: Vec<StackFrame>,
     /// Information about this `CallStack`.
     pub info: CallStackInfo,
+    /// The identifier of the thread.
+    pub thread_id: u32,
     /// The name of the thread, if known.
     pub thread_name: Option<String>,
     /// The GetLastError() value stored in the TEB.
@@ -326,10 +328,11 @@ fn json_registers(ctx: &MinidumpContext) -> serde_json::Value {
 
 impl CallStack {
     /// Create a `CallStack` with `info` and no frames.
-    pub fn with_info(info: CallStackInfo) -> CallStack {
+    pub fn with_info(id: u32, info: CallStackInfo) -> CallStack {
         CallStack {
             info,
             frames: vec![],
+            thread_id: id,
             thread_name: None,
             last_error_value: None,
         }
@@ -434,7 +437,7 @@ impl ProcessState {
 
     fn print_internal<T: Write>(&self, f: &mut T, brief: bool) -> io::Result<()> {
         writeln!(f, "Operating system: {}", self.system_info.os.long_name())?;
-        if let Some(ref ver) = self.system_info.os_version {
+        if let Some(ref ver) = self.system_info.format_os_version() {
             writeln!(f, "                  {}", ver)?;
         }
         writeln!(f, "CPU: {}", self.system_info.cpu)?;
@@ -654,7 +657,7 @@ Unknown streams encountered:
             "system_info": {
                 // Linux | Windows NT | Mac OS X
                 "os": sys.os.long_name(),
-                "os_ver": sys.os_version,
+                "os_ver": sys.format_os_version(),
                 // x86 | amd64 | arm | ppc | sparc
                 "cpu_arch": sys.cpu.to_string(),
                 "cpu_info": sys.cpu_info,
diff --git a/minidump-processor/src/processor.rs b/minidump-processor/src/processor.rs
index b980019..8fc5b8d 100644
--- a/minidump-processor/src/processor.rs
+++ b/minidump-processor/src/processor.rs
@@ -92,16 +92,17 @@ where
         .get_stream::<MinidumpSystemInfo>()
         .or(Err(ProcessError::MissingSystemInfo))?;
 
-    let mut os_version = format!(
+    let os_version = format!(
         "{}.{}.{}",
         dump_system_info.raw.major_version,
         dump_system_info.raw.minor_version,
         dump_system_info.raw.build_number
     );
-    if let Some(csd_version) = dump_system_info.csd_version() {
-        os_version.push(' ');
-        os_version.push_str(&csd_version);
-    }
+
+    let os_build = dump_system_info
+        .csd_version()
+        .map(|v| v.trim().to_owned())
+        .filter(|v| !v.is_empty());
 
     let linux_standard_base = dump.get_stream::<MinidumpLinuxLsbRelease>().ok();
     let linux_cpu_info = dump
@@ -155,6 +156,7 @@ where
     let system_info = SystemInfo {
         os: dump_system_info.os,
         os_version: Some(os_version),
+        os_build,
         cpu: dump_system_info.cpu,
         cpu_info,
         cpu_microcode_version,
@@ -223,9 +225,11 @@ where
     let mut threads = vec![];
     let mut requesting_thread = None;
     for (i, thread) in thread_list.threads.iter().enumerate() {
+        let id = thread.raw.thread_id;
+
         // If this is the thread that wrote the dump, skip processing it.
-        if dump_thread_id.is_some() && dump_thread_id.unwrap() == thread.raw.thread_id {
-            threads.push(CallStack::with_info(CallStackInfo::DumpThreadSkipped));
+        if dump_thread_id.is_some() && dump_thread_id.unwrap() == id {
+            threads.push(CallStack::with_info(id, CallStackInfo::DumpThreadSkipped));
             continue;
         }
 
@@ -250,6 +254,7 @@ where
 
         let mut stack =
             stackwalker::walk_stack(&context, stack.as_deref(), &modules, symbol_provider);
+        stack.thread_id = id;
 
         for frame in &mut stack.frames {
             // If the frame doesn't have a loaded module, try to find an unloaded module
diff --git a/minidump-processor/src/stackwalker/mod.rs b/minidump-processor/src/stackwalker/mod.rs
index bfd0146..517da63 100644
--- a/minidump-processor/src/stackwalker/mod.rs
+++ b/minidump-processor/src/stackwalker/mod.rs
@@ -198,6 +198,7 @@ where
     CallStack {
         frames,
         info,
+        thread_id: 0,
         thread_name: None,
         last_error_value: None,
     }
diff --git a/minidump-processor/src/system_info.rs b/minidump-processor/src/system_info.rs
index 576300d..54699b1 100644
--- a/minidump-processor/src/system_info.rs
+++ b/minidump-processor/src/system_info.rs
@@ -1,13 +1,20 @@
+use std::borrow::Cow;
+
 use minidump::system_info::{Cpu, Os};
 
 /// Information about the system that produced a `Minidump`.
 pub struct SystemInfo {
     /// The operating system that produced the minidump
     pub os: Os,
-    /// A string identifying the version of the operating system
+    /// A string identifying the version of the operating system.
     ///
-    /// This may look like "5.1.2600 Service Pack 2" or "10.4.8 8L2127", if present
+    /// This may look like "5.1.2600" or "10.4.8", if present
     pub os_version: Option<String>,
+    /// A string identifying the exact build of the operating system.
+    ///
+    /// This may look like "Service Pack 2" or "8L2127", if present. On Windows, this is the CSD
+    /// version, on Linux extended build information.
+    pub os_build: Option<String>,
     /// The CPU on which the dump was produced
     pub cpu: Cpu,
     /// A string further identifying the specific CPU
@@ -21,3 +28,17 @@ pub struct SystemInfo {
     /// Will be greater than one for multi-core systems.
     pub cpu_count: usize,
 }
+
+impl SystemInfo {
+    /// Returns the full available operating system version.
+    ///
+    /// Returns the version and the build, if available, otherwise just the version.
+    pub fn format_os_version(&self) -> Option<Cow<'_, str>> {
+        match (&self.os_version, &self.os_build) {
+            (Some(v), Some(b)) => Some(format!("{} {}", v, b).into()),
+            (Some(v), None) => Some(Cow::Borrowed(v)),
+            (None, Some(b)) => Some(Cow::Borrowed(b)),
+            (None, None) => None,
+        }
+    }
+}


Test patch:
diff --git a/minidump-processor/tests/test_processor.rs b/minidump-processor/tests/test_processor.rs
index 3a089c0..f4de449 100644
--- a/minidump-processor/tests/test_processor.rs
+++ b/minidump-processor/tests/test_processor.rs
@@ -53,9 +53,8 @@ fn test_processor() {
     )
     .unwrap();
     assert_eq!(state.system_info.os, Os::Windows);
-    // TODO
-    // assert_eq!(state.system_info.os_version.unwrap(),
-    // "5.1.2600 Service Pack 2");
+    assert_eq!(state.system_info.os_version.unwrap(), "5.1.2600");
+    assert_eq!(state.system_info.os_build.unwrap(), "Service Pack 2");
     assert_eq!(state.system_info.cpu, Cpu::X86);
     // TODO:
     // assert_eq!(state.system_info.cpu_info.unwrap(),