Unverified Commit 1d086a3a authored by Michael Müller's avatar Michael Müller Committed by GitHub
Browse files

Fix Option pointer bug (#570)

* Advance ptr, even when 'None'

* Revert me: Add minimal contract which reproduces bug

* Fix typo: explicitely ➜ explicitly

* Add regression test

* Improve structure

* Revert "Revert me: Add minimal contract which reproduces bug"

This reverts commit 69cd40e2.

* Fix clippy error 'result_unit_err'

* Fix clippy error 'result_unit_err'

* Fix typo: sucesful ➜ successful

* Improve comment

* Remove no-op

* Remove type aliases for V1/V2
parent d03e47d5
Pipeline #113631 canceled with stages
in 10 minutes and 57 seconds
......@@ -523,7 +523,7 @@ mod tests {
// We prevent the intermediate instance from clearing the storage preemtively by wrapping
// it inside `ManuallyDrop`. The third step will clean up the same storage region afterwards.
//
// We explicitely do not touch or assert the value of `pulled_pair.0` in order to trigger
// We explicitly do not touch or assert the value of `pulled_pair.0` in order to trigger
// the bug.
let pulled_pair: (LazyCell<i32>, i32) =
SpreadLayout::pull_spread(&mut KeyPtr::from(root_key));
......@@ -545,4 +545,57 @@ mod tests {
Ok(())
})
}
#[test]
fn regression_test_for_issue_570() -> ink_env::Result<()> {
run_test::<ink_env::DefaultEnvironment, _>(|_| {
let root_key = Key::from([0x00; 32]);
{
// Step 1: Push two valid values one after the other to contract storage.
// The first value needs to be an `Option::None` value, since the bug was
// then messing up following pointers.
let v1: Option<u32> = None;
let v2: u32 = 13;
let mut ptr = KeyPtr::from(root_key);
SpreadLayout::push_spread(&v1, &mut ptr);
SpreadLayout::push_spread(&v2, &mut ptr);
}
{
// Step 2: Pull the values from the step before.
//
// 1. Change the first values `None` to `Some(...)`.
// 2. Push the first value again to contract storage.
//
// We prevent the intermediate instance from clearing the storage preemptively
// by wrapping it inside `ManuallyDrop`. The third step will clean up the same
// storage region afterwards.
let mut ptr = KeyPtr::from(root_key);
let pulled_v1: Option<u32> = SpreadLayout::pull_spread(&mut ptr);
let mut pulled_v1 = core::mem::ManuallyDrop::new(pulled_v1);
let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr);
let pulled_v2 = core::mem::ManuallyDrop::new(pulled_v2);
assert_eq!(*pulled_v1, None);
assert_eq!(*pulled_v2, 13);
*pulled_v1 = Some(99u32);
SpreadLayout::push_spread(&*pulled_v1, &mut KeyPtr::from(root_key));
}
{
// Step 3: Pull the values again from the storage.
//
// If the bug with `Option` has been fixed in PR #520 we must be able to inspect
// the correct values for both entries.
let mut ptr = KeyPtr::from(root_key);
let pulled_v1: Option<u32> = SpreadLayout::pull_spread(&mut ptr);
let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr);
assert_eq!(pulled_v1, Some(99));
assert_eq!(pulled_v2, 13);
}
Ok(())
})
}
}
......@@ -66,6 +66,8 @@ where
<u8 as SpreadLayout>::push_spread(&(self.is_some() as u8), ptr);
if let Some(value) = self {
<T as SpreadLayout>::push_spread(value, ptr);
} else {
ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
}
}
......@@ -76,12 +78,17 @@ where
<u8 as SpreadLayout>::clear_spread(&0, ptr);
if let Some(value) = self {
<T as SpreadLayout>::clear_spread(value, ptr)
} else {
ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
}
}
fn pull_spread(ptr: &mut KeyPtr) -> Self {
match <u8 as SpreadLayout>::pull_spread(ptr) {
0u8 => None,
0u8 => {
ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
None
}
1u8 => Some(<T as SpreadLayout>::pull_spread(ptr)),
_ => unreachable!("invalid Option discriminant"),
}
......
......@@ -155,6 +155,14 @@ mod multisig_plain {
pub gas_limit: u64,
}
/// Errors that can occur upon calling this contract.
#[derive(Copy, Clone, Debug, PartialEq, Eq, scale::Encode, scale::Decode)]
#[cfg_attr(feature = "std", derive(::scale_info::TypeInfo))]
pub enum Error {
/// Returned if the call failed.
TransactionFailed,
}
#[ink(storage)]
pub struct MultisigPlain {
/// Every entry in this map represents the confirmation of an owner for a
......@@ -227,7 +235,7 @@ mod multisig_plain {
/// the output in bytes. The Option is `None` when the transaction was executed through
/// `invoke_transaction` rather than `evaluate_transaction`.
#[ink(topic)]
result: Result<Option<Vec<u8>>, ()>,
result: Result<Option<Vec<u8>>, Error>,
}
/// Emitted when an owner is added to the wallet.
......@@ -479,7 +487,10 @@ mod multisig_plain {
/// Its return value indicates whether the called transaction was successful.
/// This can be called by anyone.
#[ink(message)]
pub fn invoke_transaction(&mut self, trans_id: TransactionId) -> Result<(), ()> {
pub fn invoke_transaction(
&mut self,
trans_id: TransactionId,
) -> Result<(), Error> {
self.ensure_confirmed(trans_id);
let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID);
let result = build_call::<<Self as ::ink_lang::ContractEnv>::Env>()
......@@ -491,7 +502,7 @@ mod multisig_plain {
)
.returns::<()>()
.fire()
.map_err(|_| ());
.map_err(|_| Error::TransactionFailed);
self.env().emit_event(Execution {
transaction: trans_id,
result: result.map(|_| None),
......@@ -502,13 +513,13 @@ mod multisig_plain {
/// Evaluate a confirmed execution and return its output as bytes.
///
/// Its return value indicates whether the called transaction was successful and contains
/// its output when sucesful.
/// its output when successful.
/// This can be called by anyone.
#[ink(message)]
pub fn eval_transaction(
&mut self,
trans_id: TransactionId,
) -> Result<Vec<u8>, ()> {
) -> Result<Vec<u8>, Error> {
self.ensure_confirmed(trans_id);
let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID);
let result = build_call::<<Self as ::ink_lang::ContractEnv>::Env>()
......@@ -520,7 +531,7 @@ mod multisig_plain {
)
.returns::<ReturnType<Vec<u8>>>()
.fire()
.map_err(|_| ());
.map_err(|_| Error::TransactionFailed);
self.env().emit_event(Execution {
transaction: trans_id,
result: result.clone().map(Some),
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment