ref(symcache): FuncRecord::len must be nonzero (#323)

* fix(symcache): is_empty_function only checks size

* test(symcache): Add tests for issue #284

* ref(symcache): FuncRecord::len must be nonzero

* Addressed Jan's criticisms

* Fixed wrapping issue

symbolic-symcache/src/format.rs

@@ -4,6 +4,7 @@ use std::cmp::Ordering;
 use std::fmt;
 use std::io;
 use std::marker::PhantomData;
+use std::num::NonZeroU16;
 
 use symbolic_common::{DebugId, Uuid};
 
@@ -172,14 +173,14 @@ pub struct FileRecord {
 
 /// A function or public symbol.
 #[repr(C, packed)]
-#[derive(Default, Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub struct FuncRecord {
     /// Low bits of the address.
     pub addr_low: u32,
     /// High bits of the address.
     pub addr_high: u16,
     /// The length of the function. A value of 0xffff indicates that the size is unknown.
-    pub len: u16,
+    pub len: NonZeroU16,
     /// The line record of this function.  If it fully overlaps with an inline the record could be
     /// `~0`.
     pub line_records: Seg<LineRecord, u16>,
@@ -213,7 +214,7 @@ impl FuncRecord {
     /// If the function's [`len`](FuncRecord::len) is [`u16::MAX`], we assume it extends all the way
     /// to the end of the file.
     pub fn addr_end(&self) -> u64 {
-        match self.len {
+        match self.len.into() {
             0xffff => u64::MAX,
             len => self.addr_start() + u64::from(len),
         }

symbolic-symcache/src/writer.rs

@@ -1,6 +1,7 @@
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::io::{self, Seek, Write};
+use std::num::NonZeroU16;
 
 use fnv::{FnvHashMap, FnvHashSet};
 use num::FromPrimitive;
@@ -13,7 +14,7 @@ use crate::format;
 
 // Performs a shallow check whether this function might contain any lines.
 fn is_empty_function(function: &Function<'_>) -> bool {
-    function.lines.is_empty() && function.inlinees.is_empty()
+    function.size == 0
 }
 
 /// Performs a check whether this line has already been written in the scope of this function.
@@ -225,7 +226,7 @@ where
         for index in 0..writer.functions.len() {
             if let Some(function) = writer.functions.get(index) {
                 let address = function.original.addr;
-                let end = address + function.record.len.max(1) as u64;
+                let end = address + function.record.len.get() as u64;
 
                 // Consume all functions before and within this function. Only write the symbols
                 // before the function and drop the rest.
@@ -293,17 +294,18 @@ where
         // there is only one symbol and for the last symbol. `FuncRecord::addr_in_range` always
         // requires some address range. Since we can't possibly know the actual size, just assume
         // that the symbol is VERY large.
-        let size = match symbol.size {
-            0 => !0,
-            s => s,
+        let len = match symbol.size {
+            0 => u16::MAX,
+            s => std::cmp::min(s, 0xffff) as u16,
         };
 
+        // This unwrap cannot fail; size is nonzero by definition.
+        let len = NonZeroU16::new(len).unwrap();
+
         let record = format::FuncRecord {
             addr_low: (symbol.address & 0xffff_ffff) as u32,
             addr_high: ((symbol.address >> 32) & 0xffff) as u16,
-            // XXX: we have not seen this yet, but in theory this should be
-            // stored as multiple function records.
-            len: std::cmp::min(size, 0xffff) as u16,
+            len,
             symbol_id_low: (symbol_id & 0xffff) as u16,
             symbol_id_high: ((symbol_id >> 16) & 0xff) as u8,
             line_records: format::Seg::default(),
@@ -511,10 +513,18 @@ where
                 self.header.has_line_records = 1;
             }
 
+            let len = (end_address - start_address) as u16;
+            debug_assert_ne!(len, 0, "Function length must be positive");
+
+            let len = match NonZeroU16::new(len) {
+                Some(len) => len,
+                None => continue,
+            };
+
             let record = format::FuncRecord {
                 addr_low: (start_address & 0xffff_ffff) as u32,
                 addr_high: ((start_address >> 32) & 0xffff) as u16,
-                len: (end_address - start_address) as u16,
+                len,
                 symbol_id_low: (symbol_id & 0xffff) as u16,
                 symbol_id_high: ((symbol_id >> 16) & 0xff) as u8,
                 parent_offset: !0,

symbolic-symcache/tests/test_writer.rs

@@ -109,3 +109,48 @@ fn test_write_large_symbol_names() -> Result<(), Error> {
 
     Ok(())
 }
+
+/// This tests the fix for the bug described in
+/// https://github.com/getsentry/symbolic/issues/284#issue-726898083
+#[test]
+fn test_lookup_no_lines() -> Result<(), Error> {
+    let buffer = ByteView::open(fixture("xul.sym"))?;
+    let object = Object::parse(&buffer)?;
+
+    let mut buffer = Vec::new();
+    SymCacheWriter::write_object(&object, Cursor::new(&mut buffer))?;
+    let symcache = SymCache::parse(&buffer)?;
+    let symbols = symcache.lookup(0xc6dd98)?.collect::<Vec<_>>()?;
+
+    assert_eq!(symbols.len(), 1);
+    let name = symbols[0].function_name();
+
+    assert_eq!(
+        name,
+        "std::_Func_impl_no_alloc<`lambda at \
+        /builds/worker/checkouts/gecko/netwerk/\
+        protocol/http/HttpChannelChild.cpp:411:7',void>::_Do_call()"
+    );
+
+    Ok(())
+}
+
+/// This tests the fix for the bug described in
+/// https://github.com/getsentry/symbolic/issues/284#issuecomment-715587454.
+#[test]
+fn test_lookup_no_size() -> Result<(), Error> {
+    let buffer = ByteView::open(fixture("libgallium_dri.sym"))?;
+    let object = Object::parse(&buffer)?;
+
+    let mut buffer = Vec::new();
+    SymCacheWriter::write_object(&object, Cursor::new(&mut buffer))?;
+    let symcache = SymCache::parse(&buffer)?;
+    let symbols = symcache.lookup(0x1489adf)?.collect::<Vec<_>>()?;
+
+    assert_eq!(symbols.len(), 1);
+    let name = symbols[0].function_name();
+
+    assert_eq!(name, "nouveau_drm_screen_create");
+
+    Ok(())
+}

symbolic-testutils/fixtures/libgallium_dri.sym

@@ -0,0 +1,3 @@
+MODULE Linux x86_64 5A5F32914295A4FA22B591670A49FBEF0 libgallium_dri.so
+INFO CODE_ID 91325F5A9542FAA422B591670A49FBEF719A143E
+PUBLIC 88e650 0 nouveau_drm_screen_create

symbolic-testutils/fixtures/xul.sym

@@ -0,0 +1,3 @@
+MODULE windows x86_64 09F9D7ECF31F60E34C4C44205044422E1 xul.pdb
+INFO CODE_ID 5E8F310E69D0000 xul.dll
+FUNC c6db70 23e 0 std::_Func_impl_no_alloc<`lambda at /builds/worker/checkouts/gecko/netwerk/protocol/http/HttpChannelChild.cpp:411:7',void>::_Do_call()