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()) }