Skip to content
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

fix: fix syscall length calculation #82

Merged
merged 1 commit into from
Dec 12, 2018
Merged

fix: fix syscall length calculation #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions script/src/syscalls/load_cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ impl<'a, R: Register, M: Memory> Syscalls<R, M> for LoadCell<'a> {
let data = builder.finished_data();

let offset = machine.registers()[A2].to_usize();
let real_size = cmp::min(size, data.len() - offset);
machine.memory_mut().store64(size_addr, real_size as u64)?;
let full_size = data.len() - offset;
let real_size = cmp::min(size, full_size);
machine.memory_mut().store64(size_addr, full_size as u64)?;
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't need the length prefix before?

Copy link
Collaborator Author

@xxuejie xxuejie Dec 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This bug already exists before the refactor, but it's not used: previously we have 2 modes: ALL mode and PARTIAL mode. In ALL mode, you can either read all the data or none, there's no way to read partial data. In the other PARTIAL mode, you can read partial data, then get how much data is still available for future reading(which is the length returned here via full_size).

Throughout the building of SDK, it comes to my mind that we might not need 2 modes here, we can just stick to PARTIAL mode and keep the code in CKB simple. So in the refactoring work, I removed ALL mode and only keep PARTIAL mode. But that raises another problem: our original PARTIAL mode implementation has this bug that the returned length here is not calculated correctly, but due to our current SDK either 1) read all data using ALL mode; 2) read a given length of data using PARTIAL mode hence ignoring returned length prefix, this bug is not discovered till now.

machine
.memory_mut()
.store_bytes(addr, &data[offset..offset + real_size])?;
Expand Down
5 changes: 3 additions & 2 deletions script/src/syscalls/load_tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ impl<'a, R: Register, M: Memory> Syscalls<R, M> for LoadTx<'a> {
let data = self.tx;

let offset = machine.registers()[A2].to_usize();
let real_size = cmp::min(size, data.len() - offset);
machine.memory_mut().store64(size_addr, real_size as u64)?;
let full_size = data.len() - offset;
let real_size = cmp::min(size, full_size);
machine.memory_mut().store64(size_addr, full_size as u64)?;
machine
.memory_mut()
.store_bytes(addr, &data[offset..offset + real_size])?;
Expand Down
99 changes: 99 additions & 0 deletions script/src/syscalls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,48 @@ mod tests {
}
}

fn _test_load_tx_length(tx: &Vec<u8>) {
let mut machine = DefaultCoreMachine::<u64, SparseMemory>::default();
let size_addr = 0;
let addr = 100;

machine.registers_mut()[A0] = addr; // addr
machine.registers_mut()[A1] = size_addr; // size_addr
machine.registers_mut()[A2] = 0; // offset
machine.registers_mut()[A7] = LOAD_TX_SYSCALL_NUMBER; // syscall number

assert!(machine.memory_mut().store64(size_addr as usize, 0).is_ok());

let mut load_tx = LoadTx::new(tx);
assert!(load_tx.ecall(&mut machine).is_ok());

assert_eq!(machine.registers()[A0], SUCCESS as u64);
assert_eq!(
machine.memory_mut().load64(size_addr as usize),
Ok(tx.len() as u64)
);

machine.registers_mut()[A0] = addr; // addr
machine.registers_mut()[A2] = 100; // offset

assert!(machine.memory_mut().store64(size_addr as usize, 0).is_ok());

assert!(load_tx.ecall(&mut machine).is_ok());

assert_eq!(machine.registers()[A0], SUCCESS as u64);
assert_eq!(
machine.memory_mut().load64(size_addr as usize),
Ok(tx.len() as u64 - 100)
);
}

proptest! {
#[test]
fn test_load_tx_length(ref tx in any_with::<Vec<u8>>(size_range(1000).lift())) {
_test_load_tx_length(tx);
}
}

fn _test_load_tx_partial(tx: &Vec<u8>) {
let mut machine = DefaultCoreMachine::<u64, SparseMemory>::default();
let size_addr = 0;
Expand Down Expand Up @@ -285,6 +327,52 @@ mod tests {
}
}

fn _test_load_cell_length(data: Vec<u8>) {
let mut machine = DefaultCoreMachine::<u64, SparseMemory>::default();
let size_addr = 0;
let addr = 100;

machine.registers_mut()[A0] = addr; // addr
machine.registers_mut()[A1] = size_addr; // size_addr
machine.registers_mut()[A2] = 0; // offset
machine.registers_mut()[A3] = 0; //index
machine.registers_mut()[A4] = 0; //source: 0 input
machine.registers_mut()[A7] = LOAD_CELL_SYSCALL_NUMBER; // syscall number

let output = CellOutput::new(100, data.clone(), H256::zero(), None);
let input_cell = CellOutput::new(
100,
data.iter().rev().cloned().collect(),
H256::zero(),
None,
);
let outputs = vec![&output];
let input_cells = vec![&input_cell];
let mut load_cell = LoadCell::new(&outputs, &input_cells, &input_cell);

let mut builder = FlatBufferBuilder::new();
let fbs_offset = FbsCellOutput::build(&mut builder, &input_cell);
builder.finish(fbs_offset, None);
let input_correct_data = builder.finished_data();

assert!(machine.memory_mut().store64(size_addr as usize, 0).is_ok());

assert!(load_cell.ecall(&mut machine).is_ok());
assert_eq!(machine.registers()[A0], SUCCESS as u64);

assert_eq!(
machine.memory_mut().load64(size_addr as usize),
Ok(input_correct_data.len() as u64)
);
}

proptest! {
#[test]
fn test_load_cell_length(tx in any_with::<Vec<u8>>(size_range(1000).lift())) {
_test_load_cell_length(tx);
}
}

fn _test_load_cell_partial(data: Vec<u8>) {
let mut machine = DefaultCoreMachine::<u64, SparseMemory>::default();
let size_addr = 0;
Expand Down Expand Up @@ -464,6 +552,17 @@ mod tests {
for (i, addr) in (addr as usize..addr as usize + sha3_data.len() as usize).enumerate() {
assert_eq!(machine.memory_mut().load8(addr), Ok(sha3_data[i]));
}

machine.registers_mut()[A0] = addr; // addr
assert!(machine.memory_mut().store64(size_addr as usize, 0).is_ok());

assert!(load_cell.ecall(&mut machine).is_ok());
assert_eq!(machine.registers()[A0], SUCCESS as u64);

assert_eq!(
machine.memory_mut().load64(size_addr as usize),
Ok(sha3_data.len() as u64)
);
}

proptest! {
Expand Down
5 changes: 3 additions & 2 deletions script/src/syscalls/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ pub fn store_data<R: Register, M: Memory>(
let offset = machine.registers()[A2].to_usize();

let size = machine.memory_mut().load64(size_addr)? as usize;
let real_size = cmp::min(size, data.len() - offset);
machine.memory_mut().store64(size_addr, real_size as u64)?;
let full_size = data.len() - offset;
let real_size = cmp::min(size, full_size);
machine.memory_mut().store64(size_addr, full_size as u64)?;
machine
.memory_mut()
.store_bytes(addr, &data[offset..offset + real_size])?;
Expand Down