From c0d5c4d8d672d9b1c15f5fec491c53988ef2314b Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Wed, 18 Sep 2024 11:56:12 +0200 Subject: [PATCH] [pallet-revive] write sandbox output according to the provided output buffer length (#5743) Instead of error out if the provided output buffer is smaller than what we want to write, we can just write what fits into the output buffer instead. We already write back the actual bytes written to the in-out pointer, so contracts can check it anyways. This in turn introduces the benefit of allowing contracts to implicitly request only a portion of the returned data from calls and incantations. Which is especially beneficial for YUL as the `call` family opcodes have a return data size argument and this change removes the need to work around it in contract code. --------- Signed-off-by: xermicus --- prdoc/pr_5743.prdoc | 22 ++++ .../contracts/call_diverging_out_len.rs | 110 ++++++++++++++++++ .../fixtures/contracts/create1_with_value.rs | 4 +- substrate/frame/revive/src/lib.rs | 2 - substrate/frame/revive/src/tests.rs | 18 +++ substrate/frame/revive/src/wasm/runtime.rs | 15 +-- 6 files changed, 158 insertions(+), 13 deletions(-) create mode 100644 prdoc/pr_5743.prdoc create mode 100644 substrate/frame/revive/fixtures/contracts/call_diverging_out_len.rs diff --git a/prdoc/pr_5743.prdoc b/prdoc/pr_5743.prdoc new file mode 100644 index 000000000000..0059cbaf790c --- /dev/null +++ b/prdoc/pr_5743.prdoc @@ -0,0 +1,22 @@ +title: "[pallet-revive] write sandbox output according to the provided output buffer length" + +doc: + - audience: Runtime Dev + description: | + Instead of error out if the provided output buffer is smaller than what we want to write, + we can just write what fits into the output buffer instead. + We already write back the actual bytes written to the in-out pointer, + so contracts can check it anyways. + + This in turn introduces the benefit of allowing contracts to implicitly request only a portion + of the returned data from calls and incantations. + Which is especially beneficial for YUL as the call family opcodes have a return data size + argument and this change removes the need to work around it in contract code. + +crates: + - name: pallet-revive + bump: major + - name: pallet-revive-fixtures + bump: patch + - name: pallet-revive-uapi + bump: patch diff --git a/substrate/frame/revive/fixtures/contracts/call_diverging_out_len.rs b/substrate/frame/revive/fixtures/contracts/call_diverging_out_len.rs new file mode 100644 index 000000000000..e13162f8fb43 --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/call_diverging_out_len.rs @@ -0,0 +1,110 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! This tests that the correct output data is written when the provided +//! output buffer length is smaller than what was actually returned during +//! calls and instantiations. +//! +//! To not need an additional callee fixture, we call ourself recursively +//! and also instantiate our own code hash (constructor and recursive calls +//! always return `BUF_SIZE` bytes of data). + +#![no_std] +#![no_main] + +use common::input; +use uapi::{HostFn, HostFnImpl as api}; + +const BUF_SIZE: usize = 8; +static DATA: [u8; BUF_SIZE] = [1, 2, 3, 4, 5, 6, 7, 8]; + +/// Call `callee_address` with an output buf of size `N` +/// and expect the call output to match `expected_output`. +fn assert_call(callee_address: &[u8; 20], expected_output: [u8; BUF_SIZE]) { + let mut output_buf = [0u8; BUF_SIZE]; + let mut output_buf_capped = &mut &mut output_buf[..N]; + + api::call( + uapi::CallFlags::ALLOW_REENTRY, + callee_address, + 0u64, + 0u64, + None, + &[0u8; 32], + &[], + Some(output_buf_capped), + ) + .unwrap(); + + // The (capped) output buf should get properly resized + assert_eq!(output_buf_capped.len(), N); + assert_eq!(output_buf, expected_output); +} + +/// Instantiate this contract with an output buf of size `N` +/// and expect the instantiate output to match `expected_output`. +fn assert_instantiate(expected_output: [u8; BUF_SIZE]) { + let mut code_hash = [0; 32]; + api::own_code_hash(&mut code_hash); + + let mut output_buf = [0u8; BUF_SIZE]; + let mut output_buf_capped = &mut &mut output_buf[..N]; + + api::instantiate( + &code_hash, + 0u64, + 0u64, + None, + &[0; 32], + &[0; 32], + None, + Some(output_buf_capped), + None, + ) + .unwrap(); + + // The (capped) output buf should get properly resized + assert_eq!(output_buf_capped.len(), N); + assert_eq!(output_buf, expected_output); +} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn deploy() { + api::return_value(uapi::ReturnFlags::empty(), &DATA); +} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn call() { + let mut caller_address = [0u8; 20]; + api::caller(&mut caller_address); + + let mut callee_address = [0u8; 20]; + api::address(&mut callee_address); + + // we already recurse; return data + if caller_address == callee_address { + api::return_value(uapi::ReturnFlags::empty(), &DATA); + } + + assert_call::<0>(&callee_address, [0; 8]); + assert_call::<4>(&callee_address, [1, 2, 3, 4, 0, 0, 0, 0]); + + assert_instantiate::<0>([0; 8]); + assert_instantiate::<4>([1, 2, 3, 4, 0, 0, 0, 0]); +} diff --git a/substrate/frame/revive/fixtures/contracts/create1_with_value.rs b/substrate/frame/revive/fixtures/contracts/create1_with_value.rs index 644777aff993..c6adab828860 100644 --- a/substrate/frame/revive/fixtures/contracts/create1_with_value.rs +++ b/substrate/frame/revive/fixtures/contracts/create1_with_value.rs @@ -18,8 +18,8 @@ #![no_std] #![no_main] -use common::{input, u256_bytes}; -use uapi::{HostFn, HostFnImpl as api, ReturnErrorCode}; +use common::input; +use uapi::{HostFn, HostFnImpl as api}; #[no_mangle] #[polkavm_derive::polkavm_export] diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 1cc77a673b17..ccbc65873d02 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -467,8 +467,6 @@ pub mod pallet { InvalidCallFlags, /// The executed contract exhausted its gas limit. OutOfGas, - /// The output buffer supplied to a contract API call was too small. - OutputBufferTooSmall, /// Performing the requested transfer failed. Probably because there isn't enough /// free balance in the sender's account. TransferFailed, diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index c7185caf0efb..8a18c09e86f0 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4254,4 +4254,22 @@ mod run_tests { assert_eq!(usable_balance, value); }); } + + #[test] + fn call_diverging_out_len_works() { + let (code, _) = compile_module("call_diverging_out_len").unwrap(); + + ExtBuilder::default().existential_deposit(100).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + + // Create the contract: Constructor does nothing + let Contract { addr, .. } = + builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract(); + + // Call the contract: It will issue calls and deploys, asserting on + // correct output if the supplied output length was smaller than + // than what the callee returned. + assert_ok!(builder::call(addr).build()); + }); + } } diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 80daac8f9db3..34cce533c14a 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -651,11 +651,12 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { /// Write the given buffer and its length to the designated locations in sandbox memory and /// charge gas according to the token returned by `create_token`. - // + /// /// `out_ptr` is the location in sandbox memory where `buf` should be written to. /// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the - /// length of the buffer located at `out_ptr`. If that buffer is large enough the actual - /// `buf.len()` is written to this location. + /// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual + /// `buf.len()`, only what fits into that buffer is written to `out_ptr`. + /// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. /// /// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the /// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying @@ -681,18 +682,14 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { return Ok(()) } - let buf_len = buf.len() as u32; let len = memory.read_u32(out_len_ptr)?; - - if len < buf_len { - return Err(Error::::OutputBufferTooSmall.into()) - } + let buf_len = len.min(buf.len() as u32); if let Some(costs) = create_token(buf_len) { self.charge_gas(costs)?; } - memory.write(out_ptr, buf)?; + memory.write(out_ptr, &buf[..buf_len as usize])?; memory.write(out_len_ptr, &buf_len.encode()) }