fix: fix read syscall and sdk method (filecoin-project#584)

* fix: fix read syscall and sdk method

1. Return the correct offset (relative to the end of the logical buffer,
not relative to the end of the bytes read).
2. Don't cast "remaining" to a usize in read_block as it can be negative.
3. Add an exhaustive test.

Found by @wadealexc.

* review: make debug_asserts asserts

This costs next to nothing, and is a good safety check.

Cargo.toml

@@ -7,6 +7,7 @@ members = [
     "testing/conformance",
     "testing/integration",
     "ipld/*",
+    "testing/integration/tests/*"
 ]
 
 [profile.actor]

fvm/src/kernel/default.rs

@@ -315,22 +315,36 @@ where
     }
 
     fn block_read(&mut self, id: BlockId, offset: u32, buf: &mut [u8]) -> Result<i32> {
+        // First, find the end of the _logical_ buffer (taking the offset into account).
+        // This must fit into an i32.
+
+        // We perform operations as u64, because we know that the buffer length and offset must fit
+        // in a u32.
+        let end = i32::try_from((offset as u64) + (buf.len() as u64))
+            .map_err(|_|syscall_error!(IllegalArgument; "offset plus buffer length did not fit into an i32"))?;
+
+        // Then get the block.
         let block = self.blocks.get(id)?;
         let data = block.data();
 
+        // We start reading at this offset.
         let start = offset as usize;
 
+        // We read (block_length - start) bytes, or until we fill the buffer.
         let to_read = std::cmp::min(data.len().saturating_sub(start), buf.len());
+
+        // We can now _charge_, because we actually know how many bytes we need to read.
         self.call_manager
             .charge_gas(self.call_manager.price_list().on_block_read(to_read))?;
 
-        let end = start + to_read;
+        // Copy into the output buffer, but only if were're reading. If to_read == 0, start may be
+        // past the end of the block.
         if to_read != 0 {
-            buf[..to_read].copy_from_slice(&data[start..end]);
+            buf[..to_read].copy_from_slice(&data[start..(start + to_read)]);
         }
 
-        // Returns the difference between the end of the block, and the end of the data we've read.
-        Ok((data.len() as i32) - (end as i32))
+        // Returns the difference between the end of the block, and offset + buf.len()
+        Ok((data.len() as i32) - end)
     }
 
     fn block_stat(&mut self, id: BlockId) -> Result<BlockStat> {

sdk/src/ipld.rs

@@ -26,15 +26,14 @@ pub fn put(mh_code: u64, mh_size: u32, codec: u64, data: &[u8]) -> SyscallResult
 /// ...during the current invocation.
 pub fn get(cid: &Cid) -> SyscallResult<Vec<u8>> {
     unsafe {
-        // TODO: Check length of cid?
         let mut cid_buf = [0u8; MAX_CID_LEN];
         cid.write_bytes(&mut cid_buf[..])
             .expect("CID encoding should not fail");
         let fvm_shared::sys::out::ipld::IpldOpen { id, size, .. } =
             sys::ipld::block_open(cid_buf.as_mut_ptr())?;
         let mut block = Vec::with_capacity(size as usize);
         let remaining = sys::ipld::block_read(id, 0, block.as_mut_ptr(), size)?;
-        debug_assert_eq!(remaining, 0, "expected to read the block exactly");
+        assert_eq!(remaining, 0, "expected to read the block exactly");
         block.set_len(size as usize);
         Ok(block)
     }
@@ -60,9 +59,15 @@ pub fn get_block(id: fvm_shared::sys::BlockId, size_hint: Option<u32>) -> Syscal
                 buf.as_mut_ptr_range().end,
                 (buf.capacity() - buf.len()) as u32,
             )?;
-            debug_assert!(remaining <= 0, "should have read whole block");
+            assert!(remaining <= 0, "should have read whole block");
         }
-        buf.set_len(buf.capacity() + (remaining as usize));
+        let size = (buf.capacity() as i64) + (remaining as i64);
+        assert!(size >= 0, "size can't be negative");
+        assert!(
+            size <= buf.capacity() as i64,
+            "size shouldn't exceed capacity"
+        );
+        buf.set_len(size as usize);
     }
     Ok(buf)
 }

testing/integration/Cargo.toml

@@ -39,6 +39,7 @@ wabt = "0.10.0"
 serde = { version = "1.0", features = ["derive"] }
 fil_hello_world_actor = { path = 'tests/fil-hello-world-actor', version = '0.1' }
 fil_stack_overflow_actor = { path = 'tests/fil-stack-overflow-actor', version = '0.1' }
+fil_ipld_actor = { path = 'tests/fil-ipld-actor', version = '0.1' }
 
 [features]
 default = ["fvm/testing", "fvm_shared/testing"]

testing/integration/tests/fil-ipld-actor/Cargo.toml

@@ -0,0 +1,13 @@
+[package]
+name = "fil_ipld_actor"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+fvm_ipld_encoding = { version = "0.2.1", path = "../../../../ipld/encoding" }
+fvm_sdk = { version = "1.0.0-rc.1", path = "../../../../sdk" }
+fvm_shared = { version = "0.7.0", path = "../../../../shared" }
+
+
+[build-dependencies]
+wasm-builder = "3.0.1"

testing/integration/tests/fil-ipld-actor/build.rs

@@ -0,0 +1,12 @@
+fn main() {
+    use wasm_builder::WasmBuilder;
+    WasmBuilder::new()
+        .with_current_project()
+        .import_memory()
+        .append_to_rust_flags("-Ctarget-feature=+crt-static")
+        .append_to_rust_flags("-Cpanic=abort")
+        .append_to_rust_flags("-Coverflow-checks=true")
+        .append_to_rust_flags("-Clto=true")
+        .append_to_rust_flags("-Copt-level=z")
+        .build()
+}

testing/integration/tests/fil-ipld-actor/rust-toolchain

@@ -0,0 +1 @@
+nightly

testing/integration/tests/fil-ipld-actor/src/lib.rs

@@ -0,0 +1,127 @@
+use fvm_ipld_encoding::{to_vec, BytesSer, DAG_CBOR};
+use fvm_sdk as sdk;
+use fvm_shared::error::ExitCode;
+
+#[no_mangle]
+pub fn invoke(_: u32) -> u32 {
+    std::panic::set_hook(Box::new(|info| {
+        sdk::vm::abort(
+            ExitCode::USR_ASSERTION_FAILED.value(),
+            Some(&format!("{}", info)),
+        )
+    }));
+
+    test_read_block();
+    0
+}
+
+fn test_read_block() {
+    let test_bytes: Vec<u8> = to_vec(&BytesSer(
+        &(0..(10 << 10))
+            .map(|b| (b % 256) as u8)
+            .collect::<Vec<u8>>(),
+    ))
+    .unwrap();
+    let k = sdk::ipld::put(0xb220, 32, DAG_CBOR, &test_bytes).unwrap();
+    {
+        let block = sdk::ipld::get(&k).unwrap();
+        assert_eq!(test_bytes, block);
+    }
+
+    unsafe {
+        // Open it.
+        let k_bytes = k.to_bytes();
+        let sdk::sys::ipld::IpldOpen { codec, id, size } =
+            sdk::sys::ipld::block_open(k_bytes.as_ptr()).unwrap();
+
+        assert_eq!(test_bytes.len() as u32, size, "block has an incorrect size");
+        assert_eq!(codec, DAG_CBOR, "block has an incorrect codec");
+
+        let mut buf = vec![0u8; 2 * test_bytes.len()];
+
+        // Try reading with too much space.
+        {
+            let remaining =
+                sdk::sys::ipld::block_read(id, 0, buf.as_mut_ptr(), buf.len() as u32).unwrap();
+            assert_eq!(
+                test_bytes.len() as i32,
+                -remaining,
+                "should have over-allocated by 2x"
+            );
+            assert_eq!(
+                test_bytes,
+                buf[..test_bytes.len()],
+                "should have read entire block"
+            );
+        }
+
+        buf.fill(0);
+
+        // Try reading a slice
+        {
+            let remaining = sdk::sys::ipld::block_read(id, 10, buf.as_mut_ptr(), 10).unwrap();
+            assert_eq!(
+                remaining,
+                (test_bytes.len() - (2 * 10)) as i32,
+                "should have all but 20 bytes remaining"
+            );
+
+            assert_eq!(
+                &test_bytes[10..20],
+                &buf[..10],
+                "should have read the second 10 bytes"
+            );
+        }
+
+        // Try reading past the end.
+        {
+            let remaining =
+                sdk::sys::ipld::block_read(id, test_bytes.len() as u32 + 10, buf.as_mut_ptr(), 10)
+                    .unwrap();
+            assert_eq!(
+                remaining, -20,
+                "reading past the end of the block should work"
+            );
+        }
+
+        // Test get_block with no hint
+        assert_eq!(
+            test_bytes,
+            sdk::ipld::get_block(id, None).unwrap(),
+            "can read with no hint"
+        );
+
+        // Test get_block with a small hint
+        assert_eq!(
+            test_bytes,
+            sdk::ipld::get_block(id, Some(10)).unwrap(),
+            "can read with a small hint"
+        );
+
+        // Test get_block with an exact hint
+        assert_eq!(
+            test_bytes,
+            sdk::ipld::get_block(id, Some(test_bytes.len() as u32)).unwrap(),
+            "can read with the correct size"
+        );
+
+        // Test get_block with an oversized hint.
+        assert_eq!(
+            test_bytes,
+            sdk::ipld::get_block(id, Some(test_bytes.len() as u32 + 10)).unwrap(),
+            "can read with an over-estimated size"
+        );
+
+        // Test an offset that overflows an i32:
+        sdk::sys::ipld::block_read(id, (i32::MAX as u32) + 1, buf.as_mut_ptr(), 0)
+            .expect_err("expected it to fail");
+
+        // Test a length that overflows an i32
+        sdk::sys::ipld::block_read(id, 0, buf.as_mut_ptr(), (i32::MAX as u32) + 1)
+            .expect_err("expected it to fail");
+
+        // Test a combined length + offset that overflow an i32
+        sdk::sys::ipld::block_read(id, (i32::MAX as u32) - 10, buf.as_mut_ptr(), 20)
+            .expect_err("expected it to fail");
+    }
+}

testing/integration/tests/lib.rs

@@ -30,6 +30,9 @@ const WASM_COMPILED_PATH: &str =
 const WASM_COMPILED_PATH_OVERFLOW: &str =
     "../../target/debug/wbuild/fil_stack_overflow_actor/fil_stack_overflow_actor.compact.wasm";
 
+const WASM_COMPILED_PATH_IPLD: &str =
+    "../../target/debug/wbuild/fil_ipld_actor/fil_ipld_actor.compact.wasm";
+
 #[test]
 fn hello_world() {
     // Instantiate tester
@@ -82,6 +85,64 @@ fn hello_world() {
     assert_eq!(res.msg_receipt.exit_code.value(), 16)
 }
 
+#[test]
+fn ipld() {
+    // Instantiate tester
+    let mut tester = Tester::new(
+        NetworkVersion::V15,
+        StateTreeVersion::V4,
+        MemoryBlockstore::default(),
+    )
+    .unwrap();
+
+    let sender: [Account; 1] = tester.create_accounts().unwrap();
+
+    // Get wasm bin
+    let wasm_path = env::current_dir()
+        .unwrap()
+        .join(WASM_COMPILED_PATH_IPLD)
+        .canonicalize()
+        .unwrap();
+    let wasm_bin = std::fs::read(wasm_path).expect("Unable to read file");
+
+    // Set actor state
+    let actor_state = State::default();
+    let state_cid = tester.set_state(&actor_state).unwrap();
+
+    // Set actor
+    let actor_address = Address::new_id(10000);
+
+    tester
+        .set_actor_from_bin(&wasm_bin, state_cid, actor_address, BigInt::zero())
+        .unwrap();
+
+    // Instantiate machine
+    tester.instantiate_machine().unwrap();
+
+    // Send message
+    let message = Message {
+        from: sender[0].1,
+        to: actor_address,
+        gas_limit: 1000000000,
+        method_num: 1,
+        ..Message::default()
+    };
+
+    let res = tester
+        .executor
+        .unwrap()
+        .execute_message(message, ApplyKind::Explicit, 100)
+        .unwrap();
+
+    if !res.msg_receipt.exit_code.is_success() {
+        if let Some(info) = res.failure_info {
+            panic!("{}", info)
+        } else {
+            panic!("non-zero exit code {}", res.msg_receipt.exit_code)
+        }
+    }
+}
+
 #[test]
 fn native_stack_overflow() {
     // Instantiate tester