From fa9c2a51725c60895f7eaef40f00e3b3e3c32933 Mon Sep 17 00:00:00 2001
From: Ulrich Weigand <ulrich.weigand@de.ibm.com>
Date: Tue, 3 Nov 2020 20:54:27 +0100
Subject: [PATCH] Fix off-by-one error looking up frame info for a function
 (#2349)

The ModuleFrameInfo and FunctionInfo data structures maintain
a list of ranges via a BTreeMap.  The key to that map is one
past the end of the module/function in question.  This causes
a problem in the case of immediately adjacent ranges.  For
example, if we have two functions occupying adjacent ranges:
  A:   0-100
  B: 100-200
function A is stored with a key of 100 and B with a key of 200.

Now, when looking up the function associated with address 100,
we'd expect to find B.  However the current code:

       let (end, func) = info.functions.range(pc..).next()?;
       if pc < func.start || *end < pc {

will look up the value 100 in the map and return function A,
which will then fail the pc < func.start check in the next
line, so the result will be failure.

To fix this problem, make sure that the key used when
registering functions or modules is the address of the
last byte, not one past the end.
---
 crates/wasmtime/src/frame_info.rs | 42 ++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/crates/wasmtime/src/frame_info.rs b/crates/wasmtime/src/frame_info.rs
index 28e7b88d7e..d12464be1d 100644
--- a/crates/wasmtime/src/frame_info.rs
+++ b/crates/wasmtime/src/frame_info.rs
@@ -145,8 +145,13 @@ pub fn register(module: &CompiledModule) -> Option<GlobalFrameInfoRegistration>
         let (start, end) = unsafe {
             let ptr = (*allocated).as_ptr();
             let len = (*allocated).len();
-            (ptr as usize, ptr as usize + len)
+            // First and last byte of the function text.
+            (ptr as usize, ptr as usize + len - 1)
         };
+        // Skip empty functions.
+        if end < start {
+            continue;
+        }
         min = cmp::min(min, start);
         max = cmp::max(max, end);
         let func = FunctionInfo {
@@ -260,3 +265,38 @@ impl FrameInfo {
         (self.instr.bits() - self.func_start.bits()) as usize
     }
 }
+
+#[test]
+fn test_frame_info() -> Result<(), anyhow::Error> {
+    use crate::*;
+    let store = Store::default();
+    let module = Module::new(
+        store.engine(),
+        r#"
+            (module
+                (func (export "add") (param $x i32) (param $y i32) (result i32) (i32.add (local.get $x) (local.get $y)))
+                (func (export "sub") (param $x i32) (param $y i32) (result i32) (i32.sub (local.get $x) (local.get $y)))
+                (func (export "mul") (param $x i32) (param $y i32) (result i32) (i32.mul (local.get $x) (local.get $y)))
+                (func (export "div_s") (param $x i32) (param $y i32) (result i32) (i32.div_s (local.get $x) (local.get $y)))
+                (func (export "div_u") (param $x i32) (param $y i32) (result i32) (i32.div_u (local.get $x) (local.get $y)))
+                (func (export "rem_s") (param $x i32) (param $y i32) (result i32) (i32.rem_s (local.get $x) (local.get $y)))
+                (func (export "rem_u") (param $x i32) (param $y i32) (result i32) (i32.rem_u (local.get $x) (local.get $y)))
+            )
+         "#,
+    )?;
+    // Create an instance to ensure the frame information is registered.
+    let _ = Instance::new(&store, &module, &[])?;
+    let info = FRAME_INFO.read().unwrap();
+    for (i, alloc) in module.compiled_module().finished_functions() {
+        let (start, end) = unsafe {
+            let ptr = (**alloc).as_ptr();
+            let len = (**alloc).len();
+            (ptr as usize, ptr as usize + len)
+        };
+        for pc in start..end {
+            let frame = info.lookup_frame_info(pc).unwrap();
+            assert!(frame.func_index() == i.as_u32());
+        }
+    }
+    Ok(())
+}