Skip to content
Permalink

Comparing changes

This is a direct comparison between two commits made in this repository or its related repositories. View the default comparison for this range or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: nervosnetwork/ckb
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 1de144739154a5ef4f793caabbabb44b8e95a40b
Choose a base ref
..
head repository: nervosnetwork/ckb
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 7480a2a1f542ad34a4e5a61a56ff6ac39b0b84a6
Choose a head ref
10 changes: 10 additions & 0 deletions script/src/error.rs
Original file line number Diff line number Diff line change
@@ -197,6 +197,7 @@ impl From<TransactionScriptError> for Error {
#[cfg(test)]
mod tests {
use super::*;
use ckb_types::core::error::ARGV_TOO_LONG_TEXT;

#[test]
fn test_downcast_error_to_vm_error() {
@@ -225,4 +226,13 @@ mod tests {
);
}
}

#[test]
fn test_vm_internal_error_preserves_text() {
let vm_error = VMInternalError::Unexpected(ARGV_TOO_LONG_TEXT.to_string());
let script_error = ScriptError::VMInternalError(vm_error.clone());
let error: Error = script_error.output_type_script(177).into();

assert!(format!("{}", error).contains(ARGV_TOO_LONG_TEXT));
}
}
15 changes: 14 additions & 1 deletion script/src/syscalls/exec.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use crate::cost_model::transferred_byte_cycles;
use crate::syscalls::utils::load_c_string;
use crate::syscalls::{
Place, Source, SourceEntry, EXEC, INDEX_OUT_OF_BOUND, SLICE_OUT_OF_BOUND, WRONG_FORMAT,
Place, Source, SourceEntry, EXEC, INDEX_OUT_OF_BOUND, MAX_ARGV_LENGTH, SLICE_OUT_OF_BOUND,
WRONG_FORMAT,
};
use crate::types::Indices;
use ckb_traits::CellDataProvider;
use ckb_types::core::cell::{CellMeta, ResolvedTransaction};
use ckb_types::core::error::ARGV_TOO_LONG_TEXT;
use ckb_types::packed::{Bytes as PackedBytes, BytesVec};
use ckb_vm::Memory;
use ckb_vm::{
@@ -162,14 +164,25 @@ impl<Mac: SupportMachine, DL: CellDataProvider + Send + Sync> Syscalls<Mac> for
let argc = machine.registers()[A4].to_u64();
let mut addr = machine.registers()[A5].to_u64();
let mut argv = Vec::new();
let mut argv_length: u64 = 0;
for _ in 0..argc {
let target_addr = machine
.memory_mut()
.load64(&Mac::REG::from_u64(addr))?
.to_u64();

let cstr = load_c_string(machine, target_addr)?;
let cstr_len = cstr.len();
argv.push(cstr);

// Number of argv entries should also be considered
argv_length = argv_length
.saturating_add(8)
.saturating_add(cstr_len as u64);
if argv_length > MAX_ARGV_LENGTH {
return Err(VMError::Unexpected(ARGV_TOO_LONG_TEXT.to_string()));
}

addr += 8;
}

2 changes: 2 additions & 0 deletions script/src/syscalls/mod.rs
Original file line number Diff line number Diff line change
@@ -102,6 +102,8 @@ pub const EXEC_LOAD_ELF_V2_CYCLES_BASE: u64 = 75_000;
pub const SPAWN_EXTRA_CYCLES_BASE: u64 = 100_000;
pub const SPAWN_YIELD_CYCLES_BASE: u64 = 800;

pub const MAX_ARGV_LENGTH: u64 = 1024 * 1024;

#[derive(Debug, PartialEq, Clone, Copy, Eq)]
enum CellField {
Capacity = 0,
16 changes: 14 additions & 2 deletions script/src/syscalls/spawn.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate::syscalls::utils::load_c_string;
use crate::syscalls::{
Source, INDEX_OUT_OF_BOUND, SLICE_OUT_OF_BOUND, SOURCE_ENTRY_MASK, SOURCE_GROUP_FLAG, SPAWN,
SPAWN_EXTRA_CYCLES_BASE, SPAWN_YIELD_CYCLES_BASE,
Source, INDEX_OUT_OF_BOUND, MAX_ARGV_LENGTH, SLICE_OUT_OF_BOUND, SOURCE_ENTRY_MASK,
SOURCE_GROUP_FLAG, SPAWN, SPAWN_EXTRA_CYCLES_BASE, SPAWN_YIELD_CYCLES_BASE,
};
use crate::types::{DataPieceId, Fd, Message, SpawnArgs, TxData, VmId};
use ckb_traits::{CellDataProvider, ExtensionProvider, HeaderProvider};
use ckb_types::core::error::ARGV_TOO_LONG_TEXT;
use ckb_vm::{
machine::SupportMachine,
memory::Memory,
@@ -87,13 +88,24 @@ where
.to_u64();
let mut addr = argv_addr;
let mut argv = Vec::new();
let mut argv_length: u64 = 0;
for _ in 0..argc {
let target_addr = machine
.memory_mut()
.load64(&Mac::REG::from_u64(addr))?
.to_u64();
let cstr = load_c_string(machine, target_addr)?;
let cstr_len = cstr.len();
argv.push(cstr);

// Number of argv entries should also be considered
argv_length = argv_length
.saturating_add(8)
.saturating_add(cstr_len as u64);
if argv_length > MAX_ARGV_LENGTH {
return Err(VMError::Unexpected(ARGV_TOO_LONG_TEXT.to_string()));
}

addr = addr.wrapping_add(8);
}

5 changes: 5 additions & 0 deletions script/src/syscalls/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -7,3 +7,8 @@ pub(crate) mod utils;
mod vm_version_0;
#[path = "vm_latest/mod.rs"]
mod vm_version_1;

#[test]
fn test_max_argv_length() {
assert!(crate::syscalls::MAX_ARGV_LENGTH < u64::MAX);
}
6 changes: 5 additions & 1 deletion script/src/verify.rs
Original file line number Diff line number Diff line change
@@ -255,7 +255,11 @@ where
&self,
snapshot2_context: Arc<Mutex<Snapshot2Context<DataPieceId, TxData<DL>>>>,
) -> Spawn<DL> {
Spawn::new(self.vm_id, Arc::clone(&self.message_box), snapshot2_context)
Spawn::new(
self.vm_id,
Arc::clone(&self.message_box),
snapshot2_context,
)
}

/// Build syscall: wait
12 changes: 12 additions & 0 deletions script/src/verify_env.rs
Original file line number Diff line number Diff line change
@@ -127,4 +127,16 @@ impl TxVerifyEnv {
};
self.epoch.minimum_epoch_number_after_n_blocks(n_blocks)
}

/// Returns true if TxVerifyEnv is created to verify a submitted or
/// proposed tx
pub fn verifying_inflight_tx(&self) -> bool {
!self.verifying_committed_tx()
}

/// Returns true if TxVerifyEnv is created to verify a committed tx
/// in a block
pub fn verifying_committed_tx(&self) -> bool {
matches!(self.phase, TxVerifyPhase::Committed)
}
}
8 changes: 8 additions & 0 deletions sync/src/status.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use ckb_constant::sync::{BAD_MESSAGE_BAN_TIME, SYNC_USELESS_BAN_TIME};
use ckb_types::core::error::ARGV_TOO_LONG_TEXT;
use std::fmt::{self, Display, Formatter};
use std::time::Duration;

@@ -165,6 +166,13 @@ impl Status {
if !(400..500).contains(&(self.code as u16)) {
return None;
}
if let Some(context) = &self.context {
// TODO: it might be worthwhile to formalize all error texts
// that won't be banned.
if context.contains(ARGV_TOO_LONG_TEXT) {
return None;
}
}
match self.code {
StatusCode::GetHeadersMissCommonAncestors => Some(SYNC_USELESS_BAN_TIME),
_ => Some(BAD_MESSAGE_BAN_TIME),
2 changes: 2 additions & 0 deletions util/types/src/core/error.rs
Original file line number Diff line number Diff line change
@@ -223,3 +223,5 @@ impl OutPointError {
matches!(self, OutPointError::Unknown(_))
}
}

pub const ARGV_TOO_LONG_TEXT: &str = "@@@VM@@@UNEXPECTED@@@ARGV@@@TOOLONG@@@";
11 changes: 9 additions & 2 deletions util/types/src/core/tests/tx_pool.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use ckb_error::{ErrorKind, InternalErrorKind, SilentError as DefaultError};
use ckb_error::{ErrorKind, InternalErrorKind, OtherError, SilentError as DefaultError};

use crate::core::{
error::{OutPointError, TransactionError, TransactionErrorSource},
error::{OutPointError, TransactionError, TransactionErrorSource, ARGV_TOO_LONG_TEXT},
tx_pool::Reject,
};

@@ -99,6 +99,13 @@ fn test_if_is_malformed_tx() {
assert!(reject.is_malformed_tx());
}

{
let error_kind = ErrorKind::Script;
let error = error_kind.because(OtherError::new(ARGV_TOO_LONG_TEXT.to_string()));
let reject = Reject::Verification(error);
assert!(!reject.is_malformed_tx());
}

for error_kind in &[
InternalErrorKind::CapacityOverflow,
InternalErrorKind::DataCorrupted,
4 changes: 2 additions & 2 deletions util/types/src/core/tx_pool.rs
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@
use crate::{
core::{
self,
error::{OutPointError, TransactionError},
error::{OutPointError, TransactionError, ARGV_TOO_LONG_TEXT},
BlockNumber, Capacity, Cycle, FeeRate,
},
packed::Byte32,
@@ -71,7 +71,7 @@ fn is_malformed_from_verification(error: &Error) -> bool {
.downcast_ref::<TransactionError>()
.expect("error kind checked")
.is_malformed_tx(),
ErrorKind::Script => true,
ErrorKind::Script => !format!("{}", error).contains(ARGV_TOO_LONG_TEXT),
ErrorKind::Internal => {
error
.downcast_ref::<InternalError>()