Skip to content

feat(EOF): change oob behavior of RETURNDATALOAD and RETURNDATACOPY #1476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rakita merged 15 commits into from
Jun 8, 2024
Merged

feat(EOF): change oob behavior of RETURNDATALOAD and RETURNDATACOPY #1476

Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
709bc4a
chore(interpreter): optimisation for BYTE, SHL, SHR and SAR
jpgonzalezra May 13, 2024
7430167
test: add RETURNDATACOPY test and update RETURNDATALOAD
jpgonzalezra May 31, 2024
9051655
feat: change oob behavior of returndataload
jpgonzalezra May 31, 2024
74d9e8e
fix: stack order and name on returndatacopy test
jpgonzalezra May 31, 2024
b302cdc
feat: change oob behavior of RETURNDATACOPY
jpgonzalezra May 31, 2024
d55c796
chore: remove unused function make_me_a_table
jpgonzalezra May 31, 2024
98882f7
fix: revert interpreter check
jpgonzalezra May 31, 2024
4fc13da
chore: some tests
jpgonzalezra Jun 2, 2024
f1e2987
chore: use as_usize_or_fail! macro instead of as_usize_saturated!
jpgonzalezra Jun 2, 2024
85cc8aa
fix: add backwards compatibility
jpgonzalezra Jun 3, 2024
93c7b94
fix: collapsible-else-if
jpgonzalezra Jun 3, 2024
3d0d5d0
self review
jpgonzalezra Jun 4, 2024
eb9d6ee
Merge remote-tracking branch 'origin/main' into eof_oob
rakita Jun 8, 2024
cc93fe6
refactor code. use set_data in returndatacopy
rakita Jun 8, 2024
f87e596
remove local as it is used once
rakita Jun 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 159 additions & 24 deletions crates/interpreter/src/instructions/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,46 @@ pub fn returndatasize<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interprete
pub fn returndatacopy<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, _host: &mut H) {
check!(interpreter, BYZANTIUM);
pop!(interpreter, memory_offset, offset, len);

let len = as_usize_or_fail!(interpreter, len);
gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));

let data_offset = as_usize_saturated!(offset);
let data_end = data_offset.saturating_add(len);
if data_end > interpreter.return_data_buffer.len() {
let return_data_buffer_len = interpreter.return_data_buffer.len();

if data_offset.saturating_add(len) > return_data_buffer_len && !interpreter.is_eof {
interpreter.instruction_result = InstructionResult::OutOfOffset;
return;
}
if len != 0 {
let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
resize_memory!(interpreter, memory_offset, len);

if len == 0 {
return;
}

let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
resize_memory!(interpreter, memory_offset, len);
if data_offset < return_data_buffer_len {
let available_len = return_data_buffer_len - data_offset;
let copy_len = available_len.min(len);

interpreter.shared_memory.set(
memory_offset,
&interpreter.return_data_buffer[data_offset..data_end],
&interpreter.return_data_buffer[data_offset..data_offset + copy_len],
);

if interpreter.is_eof && copy_len < len {
interpreter
.shared_memory
.slice_mut(memory_offset + copy_len, len - copy_len)
.fill(0);
}
Copy link
Member

@rakita rakita Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if {error} is missing. Will restructure code a little bit here.

Copy link
Contributor Author

@jpgonzalezra jpgonzalezra Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, go ahead 💪

} else if interpreter.is_eof {
interpreter
.shared_memory
.slice_mut(memory_offset, len)
.fill(0);
} else {
interpreter.instruction_result = InstructionResult::OutOfOffset;
}
}

Expand All @@ -149,20 +174,20 @@ pub fn returndataload<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &m
gas!(interpreter, gas::VERYLOW);
pop_top!(interpreter, offset);
let offset_usize = as_usize_or_fail!(interpreter, offset);
// TODO EOF needs to be padded with zeros, it is not correct to fail.
/*
if offset + 32 > len(returndata buffer) the result is zero-padded
(same behavior as CALLDATALOAD).see matching behavior of RETURNDATACOPY
in Modified Behavior section.
*/
if offset_usize.saturating_add(32) > interpreter.return_data_buffer.len() {
// TODO(EOF) proper error.
interpreter.instruction_result = InstructionResult::OutOfOffset;
return;
}

*offset =
B256::from_slice(&interpreter.return_data_buffer[offset_usize..offset_usize + 32]).into();
let data = if offset_usize < interpreter.return_data_buffer.len() {
let available = interpreter.return_data_buffer.len() - offset_usize;
let mut padded = [0u8; 32];
let copy_len = available.min(32);
padded[..copy_len].copy_from_slice(
&interpreter.return_data_buffer[offset_usize..offset_usize + copy_len],
);
padded
} else {
[0u8; 32]
};

*offset = B256::from_slice(&data).into();
}

pub fn gas<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H) {
Expand All @@ -174,9 +199,9 @@ pub fn gas<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H) {
mod test {
use super::*;
use crate::{
opcode::{make_instruction_table, RETURNDATALOAD},
opcode::{make_instruction_table, RETURNDATACOPY, RETURNDATALOAD},
primitives::{bytes, Bytecode, PragueSpec},
DummyHost, Gas,
DummyHost, Gas, InstructionResult,
};

#[test]
Expand All @@ -185,7 +210,13 @@ mod test {
let mut host = DummyHost::default();

let mut interp = Interpreter::new_bytecode(Bytecode::LegacyRaw(
[RETURNDATALOAD, RETURNDATALOAD, RETURNDATALOAD].into(),
[
RETURNDATALOAD,
RETURNDATALOAD,
RETURNDATALOAD,
RETURNDATALOAD,
]
.into(),
));
interp.is_eof = true;
interp.gas = Gas::new(10000);
Expand All @@ -210,8 +241,112 @@ mod test {
);

let _ = interp.stack.pop();
let _ = interp.stack.push(U256::from(2));
let _ = interp.stack.push(U256::from(32));
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(
interp.stack.data(),
&vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])]
);

// Offset right at the boundary of the return data buffer size
let _ = interp.stack.pop();
let _ = interp
.stack
.push(U256::from(interp.return_data_buffer.len()));
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(
interp.stack.data(),
&vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])]
);
}

#[test]
fn returndatacopy() {
let table = make_instruction_table::<_, PragueSpec>();
let mut host = DummyHost::default();

let mut interp = Interpreter::new_bytecode(Bytecode::LegacyRaw(
[
RETURNDATACOPY,
RETURNDATACOPY,
RETURNDATACOPY,
RETURNDATACOPY,
RETURNDATACOPY,
RETURNDATACOPY,
]
.into(),
));
interp.is_eof = true;
interp.gas = Gas::new(10000);

interp.return_data_buffer =
bytes!("000000000000000400000000000000030000000000000002000000000000000100");
interp.shared_memory.resize(256);

// Copying within bounds
interp.stack.push(U256::from(32)).unwrap();
interp.stack.push(U256::from(0)).unwrap();
interp.stack.push(U256::from(0)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::OutOfOffset);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(
interp.shared_memory.slice(0, 32),
&interp.return_data_buffer[0..32]
);

// Copying with partial out-of-bounds (should zero pad)
interp.stack.push(U256::from(64)).unwrap();
interp.stack.push(U256::from(16)).unwrap();
interp.stack.push(U256::from(64)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(
interp.shared_memory.slice(64, 16),
&interp.return_data_buffer[16..32]
);
assert_eq!(&interp.shared_memory.slice(80, 48), &[0u8; 48]);

// Completely out-of-bounds (should be all zeros)
interp.stack.push(U256::from(32)).unwrap();
interp.stack.push(U256::from(96)).unwrap();
interp.stack.push(U256::from(128)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(&interp.shared_memory.slice(128, 32), &[0u8; 32]);

// Large offset
interp.stack.push(U256::from(32)).unwrap();
interp.stack.push(U256::MAX).unwrap();
interp.stack.push(U256::from(0)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(&interp.shared_memory.slice(0, 32), &[0u8; 32]);

// Offset just before the boundary of the return data buffer size
interp.stack.push(U256::from(32)).unwrap();
interp
.stack
.push(U256::from(interp.return_data_buffer.len() - 32))
.unwrap();
interp.stack.push(U256::from(0)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(
interp.shared_memory.slice(0, 32),
&interp.return_data_buffer[interp.return_data_buffer.len() - 32..]
);

// Offset right at the boundary of the return data buffer size
interp.stack.push(U256::from(32)).unwrap();
interp
.stack
.push(U256::from(interp.return_data_buffer.len()))
.unwrap();
interp.stack.push(U256::from(0)).unwrap();
interp.step(&table, &mut host);
assert_eq!(interp.instruction_result, InstructionResult::Continue);
assert_eq!(&interp.shared_memory.slice(0, 32), &[0u8; 32]);
}
}