From 2afc4743383686411941fba431f1bf3a8271c059 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 25 Feb 2025 09:49:54 -0700 Subject: [PATCH] Addressing post-hoc PR feedback on #174 (#197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add ECC & myself (Greg Pfeil) as authors * Have the Rust impl correctly report “high s” sigs There is a bug in the C++ side where the error is not set correctly on a “high s” signature. The Rust side had mirrored this bug, but this eliminates the bug in the Rust. * Remove extra byte from sig before low-s check This doesn’t seem to have any effect on the semantics, as the DER-formatted signature includes lengths that ensure it will ignore extra bytes, but the C++ code removes the extra byte, so the Rust should as well. * Change some comments Co-authored-by: Daira-Emma Hopwood * Appease `rustfmt` * Have OP_DUP match the C++ impl more closely * Address the second half of @daira’s #174 review * Eliminate mutation from `Opcode` parsing This now splits slices and returns the remaining pieces rather than modifying the arguments. * Remove obsolete comment * Address PR comments * Address additional comments on #174 --------- Co-authored-by: Daira-Emma Hopwood --- Cargo.toml | 7 +- src/interpreter.rs | 231 +++++++++++++++++++++----------------------- src/lib.rs | 7 +- src/script.rs | 209 ++++++++++++++++----------------------- src/script_error.rs | 3 +- 5 files changed, 203 insertions(+), 254 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8de77b4b8..534af16eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,12 @@ [package] name = "zcash_script" version = "0.2.0" -authors = ["Tamas Blummer ", "Zcash Foundation "] +authors = [ + "Electric Coin Company ", + "Greg Pfeil ", + "Tamas Blummer ", + "Zcash Foundation ", +] license = "Apache-2.0" readme = "README.md" build = "build.rs" diff --git a/src/interpreter.rs b/src/interpreter.rs index 71cfeab84..148cf055b 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -372,13 +372,15 @@ fn is_low_der_signature(vch_sig: &ValType) -> Result { // https://bitcoin.stackexchange.com/a/12556: // Also note that inside transaction signatures, an extra hashtype byte // follows the actual signature data. - let vch_sig_copy = vch_sig.clone(); + let (_, vch_sig_copy) = vch_sig + .split_last() + .expect("`is_valid_signature_encoding` checks that the length is at least 9"); // If the S value is above the order of the curve divided by two, its // complement modulo the order could have been used instead, which is // one byte shorter when encoded correctly. // FIXME: This can return `false` without setting an error, which is not the expectation of the // caller. - Ok(PubKey::check_low_s(&vch_sig_copy)) + Ok(PubKey::check_low_s(vch_sig_copy)) } fn is_defined_hashtype_signature(vch_sig: &ValType) -> bool { @@ -392,24 +394,22 @@ fn is_defined_hashtype_signature(vch_sig: &ValType) -> bool { fn check_signature_encoding( vch_sig: &Vec, flags: VerificationFlags, -) -> Result { +) -> Result<(), ScriptError> { // Empty signature. Not strictly DER encoded, but allowed to provide a // compact way to provide an invalid signature for use with CHECK(MULTI)SIG if vch_sig.is_empty() { - return Ok(true); + return Ok(()); }; if !is_valid_signature_encoding(vch_sig) { return set_error(ScriptError::SigDER); } else if flags.contains(VerificationFlags::LowS) && !is_low_der_signature(vch_sig)? { - // TODO: The C++ impl claims that `serror` is set here, but it isn’t, so this eventually - // ends up as `UnknownError`, when it should probably be `SigHighS`. - return Ok(false); + return set_error(ScriptError::SigHighS); } else if flags.contains(VerificationFlags::StrictEnc) && !is_defined_hashtype_signature(vch_sig) { return set_error(ScriptError::SigHashType); }; - Ok(true) + Ok(()) } fn check_pub_key_encoding(vch_sig: &ValType, flags: VerificationFlags) -> Result<(), ScriptError> { @@ -421,7 +421,7 @@ fn check_pub_key_encoding(vch_sig: &ValType, flags: VerificationFlags) -> Result set_success(()) } -fn check_minimal_push(data: &ValType, opcode: PushValue) -> bool { +fn check_minimal_push(data: &[u8], opcode: PushValue) -> bool { if data.is_empty() { // Could have used OP_0. return opcode == OP_0; @@ -449,7 +449,7 @@ pub fn eval_script( script: &Script, flags: VerificationFlags, checker: &dyn SignatureChecker, -) -> Result { +) -> Result<(), ScriptError> { let bn_zero = ScriptNum::from(0); let bn_one = ScriptNum::from(1); let vch_false: ValType = vec![]; @@ -461,7 +461,6 @@ pub fn eval_script( } let mut pc = script.0; - let mut vch_push_value = vec![]; // We keep track of how many operations have executed so far to prevent // expensive-to-verify scripts @@ -484,7 +483,8 @@ pub fn eval_script( // // Read instruction // - let opcode = Script::get_op2(&mut pc, &mut vch_push_value)?; + let (opcode, vch_push_value, new_pc) = Script::get_op2(pc)?; + pc = new_pc; if vch_push_value.len() > MAX_SCRIPT_ELEMENT_SIZE { return set_error(ScriptError::PushSize); } @@ -506,10 +506,10 @@ pub fn eval_script( } _ => { if pv <= OP_PUSHDATA4 { - if require_minimal && !check_minimal_push(&vch_push_value, pv) { + if require_minimal && !check_minimal_push(vch_push_value, pv) { return set_error(ScriptError::MinimalData); } - stack.push_back(vch_push_value.clone()); + stack.push_back(vch_push_value.to_vec()); } else { return set_error(ScriptError::BadOpcode); } @@ -552,10 +552,16 @@ pub fn eval_script( OP_NOP => (), OP_CHECKLOCKTIMEVERIFY => { - // This was originally OP_NOP2 but has been repurposed - // for OP_CHECKLOCKTIMEVERIFY. So, we should act based - // on whether or not CLTV has been activated in a soft - // fork. + // https://zips.z.cash/protocol/protocol.pdf#bips : + // + // The following BIPs apply starting from the Zcash genesis block, + // i.e. any activation rules or exceptions for particular blocks in + // the Bitcoin block chain are to be ignored: [BIP-16], [BIP-30], + // [BIP-65], [BIP-66]. + // + // So BIP 65, which defines CHECKLOCKTIMEVERIFY, is in practice always + // enabled, and this `if` branch is dead code. In zcashd see + // https://github.com/zcash/zcash/blob/a3435336b0c561799ac6805a27993eca3f9656df/src/main.cpp#L3151 if !flags.contains(VerificationFlags::CHECKLOCKTIMEVERIFY) { if flags.contains(VerificationFlags::DiscourageUpgradableNOPs) { return set_error(ScriptError::DiscourageUpgradableNOPs); @@ -596,8 +602,8 @@ pub fn eval_script( } } - OP_NOP1 | OP_NOP3 | OP_NOP4 | OP_NOP5 - | OP_NOP6 | OP_NOP7 | OP_NOP8 | OP_NOP9 | OP_NOP10 => { + OP_NOP1 | OP_NOP3 | OP_NOP4 | OP_NOP5 | OP_NOP6 | OP_NOP7 | OP_NOP8 + | OP_NOP9 | OP_NOP10 => { // Do nothing, though if the caller wants to prevent people from using // these NOPs (as part of a standard tx rule, for example) they can // enable `DiscourageUpgradableNOPs` to turn these opcodes into errors. @@ -606,8 +612,7 @@ pub fn eval_script( } } - OP_IF - | OP_NOTIF => { + OP_IF | OP_NOTIF => { // if [statements] [else [statements]] endif let mut value = false; if exec { @@ -751,7 +756,8 @@ pub fn eval_script( OP_DEPTH => { // -- stacksize - let bn = ScriptNum::try_from(stack.size()).map_err(|_| ScriptError::StackSize)?; + let bn = ScriptNum::try_from(stack.size()) + .map_err(|_| ScriptError::StackSize)?; stack.push_back(bn.getvch()) } @@ -769,9 +775,8 @@ pub fn eval_script( return set_error(ScriptError::InvalidStackOperation); } - let a = stack.pop()?; - stack.push_back(a.clone()); - stack.push_back(a); + let vch = stack.top(-1)?; + stack.push_back(vch.clone()); } OP_NIP => { @@ -791,22 +796,28 @@ pub fn eval_script( stack.push_back(vch.clone()); } - OP_PICK - | OP_ROLL => { + OP_PICK | OP_ROLL => { // (xn ... x2 x1 x0 n - xn ... x2 x1 x0 xn) // (xn ... x2 x1 x0 n - ... x2 x1 x0 xn) if stack.size() < 2 { return set_error(ScriptError::InvalidStackOperation); } - let n = - u16::try_from(ScriptNum::new(stack.top(-1)?, require_minimal, None)?) - .map_err(|_| ScriptError::InvalidStackOperation)?; + let n = u16::try_from(ScriptNum::new( + stack.top(-1)?, + require_minimal, + None, + )?) + .map_err(|_| ScriptError::InvalidStackOperation)?; stack.pop()?; if usize::from(n) >= stack.size() { return set_error(ScriptError::InvalidStackOperation); } - let vch: ValType = - stack.top(-isize::try_from(n).map_err(|_| ScriptError::InvalidStackOperation)? - 1)? + let vch: ValType = stack + .top( + -isize::try_from(n) + .map_err(|_| ScriptError::InvalidStackOperation)? + - 1, + )? .clone(); if op == OP_ROLL { stack.erase(stack.end() - usize::from(n) - 1, None); @@ -842,61 +853,47 @@ pub fn eval_script( stack.insert(stack.end() - 2, vch) } - OP_SIZE => { // (in -- in size) if stack.size() < 1 { return set_error(ScriptError::InvalidStackOperation); } - let bn = - ScriptNum::try_from(stack.top(-1)?.len()).map_err(|_| ScriptError::PushSize)?; + let bn = ScriptNum::try_from(stack.top(-1)?.len()) + .expect("stack element size fits in ScriptNum"); stack.push_back(bn.getvch()) } - // // Bitwise logic // - OP_EQUAL - | OP_EQUALVERIFY - // | OP_NOTEQUAL // use OP_NUMNOTEQUAL - => { - // (x1 x2 - bool) - if stack.size() < 2 { - return set_error(ScriptError::InvalidStackOperation); - } - let vch1 = stack.top(-2)?.clone(); - let vch2 = stack.top(-1)?.clone(); - let equal = vch1 == vch2; - // OP_NOTEQUAL is disabled because it would be too easy to say - // something like n != 1 and have some wiseguy pass in 1 with extra - // zero bytes after it (numerically, 0x01 == 0x0001 == 0x000001) - //if op == OP_NOTEQUAL { - // fEqual = !fEqual; - //} - stack.pop()?; - stack.pop()?; - stack.push_back(if equal { vch_true.clone() } else { vch_false.clone() }); - if op == OP_EQUALVERIFY - { - if equal { - stack.pop()?; - } else { - return set_error(ScriptError::EqualVerify); - } + OP_EQUAL | OP_EQUALVERIFY => { + // (x1 x2 - bool) + if stack.size() < 2 { + return set_error(ScriptError::InvalidStackOperation); + } + let vch1 = stack.top(-2)?.clone(); + let vch2 = stack.top(-1)?.clone(); + let equal = vch1 == vch2; + stack.pop()?; + stack.pop()?; + stack.push_back(if equal { + vch_true.clone() + } else { + vch_false.clone() + }); + if op == OP_EQUALVERIFY { + if equal { + stack.pop()?; + } else { + return set_error(ScriptError::EqualVerify); } } - + } // // Numeric // - OP_1ADD - | OP_1SUB - | OP_NEGATE - | OP_ABS - | OP_NOT - | OP_0NOTEQUAL => { + OP_1ADD | OP_1SUB | OP_NEGATE | OP_ABS | OP_NOT | OP_0NOTEQUAL => { // (in -- out) if stack.size() < 1 { return set_error(ScriptError::InvalidStackOperation); @@ -939,11 +936,9 @@ pub fn eval_script( let bn1 = ScriptNum::new(stack.top(-2)?, require_minimal, None)?; let bn2 = ScriptNum::new(stack.top(-1)?, require_minimal, None)?; let bn = match op { - OP_ADD => - bn1 + bn2, + OP_ADD => bn1 + bn2, - OP_SUB => - bn1 - bn2, + OP_SUB => bn1 - bn2, OP_BOOLAND => ScriptNum::from(bn1 != bn_zero && bn2 != bn_zero), OP_BOOLOR => ScriptNum::from(bn1 != bn_zero || bn2 != bn_zero), @@ -954,8 +949,20 @@ pub fn eval_script( OP_GREATERTHAN => ScriptNum::from(bn1 > bn2), OP_LESSTHANOREQUAL => ScriptNum::from(bn1 <= bn2), OP_GREATERTHANOREQUAL => ScriptNum::from(bn1 >= bn2), - OP_MIN => if bn1 < bn2 { bn1 } else { bn2 }, - OP_MAX => if bn1 > bn2 { bn1 } else { bn2 }, + OP_MIN => { + if bn1 < bn2 { + bn1 + } else { + bn2 + } + } + OP_MAX => { + if bn1 > bn2 { + bn1 + } else { + bn2 + } + } _ => panic!("invalid opcode"), }; stack.pop()?; @@ -993,11 +1000,7 @@ pub fn eval_script( // // Crypto // - OP_RIPEMD160 - | OP_SHA1 - | OP_SHA256 - | OP_HASH160 - | OP_HASH256 => { + OP_RIPEMD160 | OP_SHA1 | OP_SHA256 | OP_HASH160 | OP_HASH256 => { // (in -- hash) if stack.size() < 1 { return set_error(ScriptError::InvalidStackOperation); @@ -1021,8 +1024,7 @@ pub fn eval_script( stack.push_back(vch_hash) } - OP_CHECKSIG - | OP_CHECKSIGVERIFY => { + OP_CHECKSIG | OP_CHECKSIGVERIFY => { // (sig pubkey -- bool) if stack.size() < 2 { return set_error(ScriptError::InvalidStackOperation); @@ -1031,10 +1033,7 @@ pub fn eval_script( let vch_sig = stack.top(-2)?.clone(); let vch_pub_key = stack.top(-1)?.clone(); - if !check_signature_encoding(&vch_sig, flags)? { - //serror is set - return Ok(false); - } + check_signature_encoding(&vch_sig, flags)?; check_pub_key_encoding(&vch_pub_key, flags)?; let success = checker.check_sig(&vch_sig, &vch_pub_key, script); @@ -1054,8 +1053,7 @@ pub fn eval_script( } } - OP_CHECKMULTISIG - | OP_CHECKMULTISIGVERIFY => { + OP_CHECKMULTISIG | OP_CHECKMULTISIGVERIFY => { // ([sig ...] num_of_signatures [pubkey ...] num_of_pubkeys -- bool) // NB: This is guaranteed u8-safe, because we are limited to 20 keys and @@ -1066,12 +1064,16 @@ pub fn eval_script( return set_error(ScriptError::InvalidStackOperation); }; - let mut keys_count = - u8::try_from(ScriptNum::new(stack.top(-isize::from(i))?, require_minimal, None)?) - .map_err(|_| ScriptError::PubKeyCount)?; + let mut keys_count = u8::try_from(ScriptNum::new( + stack.top(-isize::from(i))?, + require_minimal, + None, + )?) + .map_err(|_| ScriptError::PubKeyCount)?; if keys_count > 20 { return set_error(ScriptError::PubKeyCount); }; + assert!(op_count <= 201); op_count += keys_count; if op_count > 201 { return set_error(ScriptError::OpCount); @@ -1083,12 +1085,16 @@ pub fn eval_script( return set_error(ScriptError::InvalidStackOperation); } - let mut sigs_count = - u8::try_from(ScriptNum::new(stack.top(-isize::from(i))?, require_minimal, None)?) - .map_err(|_| ScriptError::SigCount)?; + let mut sigs_count = u8::try_from(ScriptNum::new( + stack.top(-isize::from(i))?, + require_minimal, + None, + )?) + .map_err(|_| ScriptError::SigCount)?; if sigs_count > keys_count { return set_error(ScriptError::SigCount); }; + assert!(i <= 22); i += 1; let mut isig = i; i += sigs_count; @@ -1104,10 +1110,7 @@ pub fn eval_script( // Note how this makes the exact order of pubkey/signature evaluation // distinguishable by CHECKMULTISIG NOT if the STRICTENC flag is set. // See the script_(in)valid tests for details. - if !check_signature_encoding(vch_sig, flags)? { - // serror is set - return Ok(false); - }; + check_signature_encoding(vch_sig, flags)?; check_pub_key_encoding(vch_pub_key, flags)?; // Check signature @@ -1129,11 +1132,7 @@ pub fn eval_script( } // Clean up stack of actual arguments - while { - let res = i > 1; - i -= 1; - res - } { + for _ in 1..i { stack.pop()?; } @@ -1186,7 +1185,7 @@ pub fn eval_script( return set_error(ScriptError::UnbalancedConditional); } - set_success(true) + set_success(()) } /// All signature hashes are 32 bytes, since they are either: @@ -1228,7 +1227,7 @@ impl SignatureChecker for CallbackTransactionSignatureChecker<'_> { } fn check_lock_time(&self, lock_time: &ScriptNum) -> bool { - // There are two times of nLockTime: lock-by-blockheight + // There are two kinds of nLockTime: lock-by-blockheight // and lock-by-blocktime, distinguished by whether // nLockTime < LOCKTIME_THRESHOLD. // @@ -1270,17 +1269,11 @@ pub fn verify_script( let mut stack = Stack(Vec::new()); let mut stack_copy = Stack(Vec::new()); - if !eval_script(&mut stack, script_sig, flags, checker)? { - // serror is set - return set_error(ScriptError::UnknownError); - } + eval_script(&mut stack, script_sig, flags, checker)?; if flags.contains(VerificationFlags::P2SH) { stack_copy = stack.clone() } - if !eval_script(&mut stack, script_pub_key, flags, checker)? { - // serror is set - return set_error(ScriptError::UnknownError); - } + eval_script(&mut stack, script_pub_key, flags, checker)?; if stack.empty() { return set_error(ScriptError::EvalFalse); } @@ -1307,10 +1300,7 @@ pub fn verify_script( let pub_key_2 = Script(pub_key_serialized.as_slice()); stack.pop()?; - if !eval_script(&mut stack, &pub_key_2, flags, checker)? { - // serror is set - return set_error(ScriptError::UnknownError); - } + eval_script(&mut stack, &pub_key_2, flags, checker)?; if stack.empty() { return set_error(ScriptError::EvalFalse); } @@ -1323,8 +1313,7 @@ pub fn verify_script( // as the non-P2SH evaluation of a P2SH script will obviously not result in // a clean stack (the P2SH inputs remain). if flags.contains(VerificationFlags::CleanStack) { - // Disallow CLEANSTACK without P2SH, as otherwise a switch CLEANSTACK->P2SH+CLEANSTACK - // would be possible, which is not a softfork (and P2SH should be one). + // Disallow CLEANSTACK without P2SH, because Bitcoin did. assert!(flags.contains(VerificationFlags::P2SH)); if stack.size() != 1 { return set_error(ScriptError::CleanStack); diff --git a/src/lib.rs b/src/lib.rs index c5fb4626a..e03dbe302 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -128,7 +128,9 @@ extern "C" fn sighash_callback( // SAFETY: `ctx` is a valid `SighashCalculator` constructed in `verify_callback` // which forwards it to the `CallbackTransactionSignatureChecker`. let callback = unsafe { *(ctx as *const SighashCalculator) }; - // We don’t need to handle strictness here, because … something + // We don’t need to handle strictness here, because it is checked (when necessary) by + // `CheckSignatureEncoding` before `CallbackTransactionSignatureChecker` calls this callback. + // And we don’t have access to the flags here to determine if it should be checked. if let Some(sighash) = HashType::from_bits(hash_type, false) .ok() .and_then(|ht| callback(script_code_vec, ht)) @@ -286,8 +288,7 @@ impl ZcashScript for CxxRustComparisonInterpreter { ); if rust.map_err(normalize_error) != cxx { // probably want to distinguish between - // - C++ succeeding when Rust fails (bad), - // - Rust succeeding when C++ fals (worse), and + // - one succeeding when the other fails (bad), and // - differing error codes (maybe not bad). warn!( "The Rust Zcash Script interpreter had a different result ({:?}) from the C++ one ({:?}).", diff --git a/src/script.rs b/src/script.rs index a11c0fc13..ab24e301d 100644 --- a/src/script.rs +++ b/src/script.rs @@ -156,7 +156,7 @@ pub enum Operation { // expansion OP_NOP1 = 0xb0, - OP_NOP2 = 0xb1, + OP_CHECKLOCKTIMEVERIFY = 0xb1, OP_NOP3 = 0xb2, OP_NOP4 = 0xb3, OP_NOP5 = 0xb4, @@ -172,8 +172,6 @@ pub enum Operation { use Operation::*; -pub const OP_CHECKLOCKTIMEVERIFY: Operation = OP_NOP2; - impl From for u8 { fn from(value: Opcode) -> Self { match value { @@ -292,15 +290,14 @@ impl ScriptNum { // If the most-significant-byte - excluding the sign bit - is zero // then we're not minimal. Note how this test also rejects the // negative-zero encoding, 0x80. - if (vch.last().unwrap_or_else(|| unreachable!()) & 0x7F) == 0 { + if (vch.last().expect("not empty") & 0x7F) == 0 { // One exception: if there's more than one byte and the most // significant bit of the second-most-significant-byte is set - // it would conflict with the sign bit. An example of this case - // is +-255, which encode to 0xff00 and 0xff80 respectively. - // (big-endian). - if vch.len() <= 1 { - return Err(ScriptNumError::NegativeZero); - } else if (vch[vch.len() - 2] & 0x80) == 0 { + // then it would have conflicted with the sign bit if one + // fewer byte were used, and so such encodings are minimal. + // An example of this is +-255, which have minimal encodings + // [0xff, 0x00] and [0xff, 0x80] respectively. + if vch.len() <= 1 || (vch[vch.len() - 2] & 0x80) == 0 { return Err(ScriptNumError::NonMinimalEncoding); } } @@ -328,42 +325,35 @@ impl ScriptNum { } if *value == i64::MIN { - // The code below is buggy, and produces the "wrong" result for - // INT64_MIN. To avoid undefined behavior while attempting to - // negate a value of INT64_MIN, we intentionally return the result - // that the code below would produce on an x86_64 system. + // The code below was based on buggy C++ code, that produced the + // "wrong" result for INT64_MIN. In that case we intentionally return + // the result that the C++ code as compiled for zcashd (with `-fwrapv`) + // originally produced on an x86_64 system. return vec![0, 0, 0, 0, 0, 0, 0, 128, 128]; } let mut result = Vec::new(); let neg = *value < 0; - let mut absvalue = value.abs(); + let mut absvalue = value.unsigned_abs(); while absvalue != 0 { - result.push( - (absvalue & 0xff) - .try_into() - .unwrap_or_else(|_| unreachable!()), - ); + result.push((absvalue & 0xff).try_into().expect("fits in u8")); absvalue >>= 8; } - // - If the most significant byte is >= 0x80 and the value is positive, push a - // new zero-byte to make the significant byte < 0x80 again. - - // - If the most significant byte is >= 0x80 and the value is negative, push a - // new 0x80 byte that will be popped off when converting to an integral. + // - If the most significant byte is >= 0x80 and the value is positive, push a + // new zero-byte to make the significant byte < 0x80 again. + // - If the most significant byte is >= 0x80 and the value is negative, push a + // new 0x80 byte that will be popped off when converting to an integral. + // - If the most significant byte is < 0x80 and the value is negative, add 0x80 + // to it, since it will be subtracted and interpreted as a negative when + // converting to an integral. - // - If the most significant byte is < 0x80 and the value is negative, add - // 0x80 to it, since it will be subtracted and interpreted as a negative when - // converting to an integral. - - if result.last().map_or(true, |last| last & 0x80 != 0) { + let result_back = result.last_mut().expect("not empty"); + if *result_back & 0x80 != 0 { result.push(if neg { 0x80 } else { 0 }); } else if neg { - if let Some(last) = result.last_mut() { - *last |= 0x80; - } + *result_back |= 0x80; } result @@ -374,14 +364,15 @@ impl ScriptNum { None => Ok(0), Some(vch_back) => { if *vch == vec![0, 0, 0, 0, 0, 0, 0, 128, 128] { - // On an x86_64 system, the code below would actually decode the buggy - // INT64_MIN encoding correctly. However in this case, it would be - // performing left shifts of a signed type by 64, which has undefined - // behavior. + // Match the behaviour of the C++ code, which special-cased this + // encoding to avoid an undefined shift of a signed type by 64 bits. return Ok(i64::MIN); }; - // Guard against undefined behavior. INT64_MIN is the only allowed 9-byte encoding. + // Ensure defined behaviour (in Rust, left shift of `i64` by 64 bits + // is an arithmetic overflow that may panic or give an unspecified + // result). The above encoding of `i64::MIN` is the only allowed + // 9-byte encoding. if vch.len() > 8 { return Err(ScriptNumError::Overflow { max_num_size: 8, @@ -457,13 +448,11 @@ impl Add for ScriptNum { type Output = Self; fn add(self, other: Self) -> Self { - let rhs = other.0; - assert!( - rhs == 0 - || (rhs > 0 && self.0 <= i64::MAX - rhs) - || (rhs < 0 && self.0 >= i64::MIN - rhs) - ); - Self(self.0 + rhs) + Self( + self.0 + .checked_add(other.0) + .expect("caller should avoid overflow"), + ) } } @@ -471,13 +460,11 @@ impl Sub for ScriptNum { type Output = Self; fn sub(self, other: Self) -> Self { - let rhs = other.0; - assert!( - rhs == 0 - || (rhs > 0 && self.0 >= i64::MIN + rhs) - || (rhs < 0 && self.0 <= i64::MAX + rhs) - ); - Self(self.0 - rhs) + Self( + self.0 + .checked_sub(other.0) + .expect("caller should avoid underflow"), + ) } } @@ -495,85 +482,50 @@ impl Neg for ScriptNum { pub struct Script<'a>(pub &'a [u8]); impl Script<'_> { - pub fn get_op(script: &mut &[u8]) -> Result { - Self::get_op2(script, &mut vec![]) + pub fn get_op(script: &[u8]) -> Result<(Opcode, &[u8]), ScriptError> { + Self::get_op2(script).map(|(op, _, remainder)| (op, remainder)) } - pub fn get_op2(script: &mut &[u8], buffer: &mut Vec) -> Result { - if script.is_empty() { - return Err(ScriptError::ReadError { - expected_bytes: 1, - available_bytes: 0, - }); - } - - // Empty the provided buffer, if any - buffer.truncate(0); - - let leading_byte = Opcode::from(script[0]); - *script = &script[1..]; - - Ok(match leading_byte { - Opcode::PushValue(pv) => match pv { - OP_PUSHDATA1 | OP_PUSHDATA2 | OP_PUSHDATA4 => { - let read_le = |script: &mut &[u8], needed_bytes: usize| { - if script.len() < needed_bytes { - Err(ScriptError::ReadError { - expected_bytes: needed_bytes, - available_bytes: script.len(), - }) - } else { - let mut size = 0; - for i in (0..needed_bytes).rev() { - size <<= 8; - size |= usize::from(script[i]); - } - *script = &script[needed_bytes..]; - Ok(size) - } - }; - - let size = match pv { - OP_PUSHDATA1 => read_le(script, 1), - OP_PUSHDATA2 => read_le(script, 2), - OP_PUSHDATA4 => read_le(script, 4), - _ => unreachable!(), - }?; - - if script.len() < size { - return Err(ScriptError::ReadError { - expected_bytes: size, - available_bytes: script.len(), - }); - } - - buffer.extend(&script[0..size]); - *script = &script[size..]; - - leading_byte - } - // OP_0/OP_FALSE doesn't actually push a constant 0 onto the stack but - // pushes an empty array. (Thus we leave the buffer truncated to 0 length) - OP_0 => leading_byte, - PushdataBytelength(size_byte) => { - let size = size_byte.into(); - - if script.len() < size { - return Err(ScriptError::ReadError { - expected_bytes: size, - available_bytes: script.len(), - }); - } + fn split_value(script: &[u8], needed_bytes: usize) -> Result<(&[u8], &[u8]), ScriptError> { + script + .split_at_checked(needed_bytes) + .ok_or(ScriptError::ReadError { + expected_bytes: needed_bytes, + available_bytes: script.len(), + }) + } - buffer.extend(&script[0..size]); - *script = &script[size..]; + /// First splits `size_size` bytes to determine the size of the value to read, then splits the + /// value. + fn split_tagged_value(script: &[u8], size_size: usize) -> Result<(&[u8], &[u8]), ScriptError> { + Script::split_value(script, size_size).and_then(|(bytes, script)| { + let mut size = 0; + for byte in bytes.iter().rev() { + size <<= 8; + size |= usize::from(*byte); + } + Script::split_value(script, size) + }) + } - leading_byte + pub fn get_op2(script: &[u8]) -> Result<(Opcode, &[u8], &[u8]), ScriptError> { + match script.split_first() { + None => Err(ScriptError::ReadError { + expected_bytes: 1, + available_bytes: 0, + }), + Some((leading_byte, script)) => match Opcode::from(*leading_byte) { + op @ Opcode::PushValue(pv) => match pv { + OP_PUSHDATA1 => Script::split_tagged_value(script, 1), + OP_PUSHDATA2 => Script::split_tagged_value(script, 2), + OP_PUSHDATA4 => Script::split_tagged_value(script, 4), + PushdataBytelength(size_byte) => Script::split_value(script, size_byte.into()), + _ => Ok((&[][..], script)), } - _ => leading_byte, + .map(|(value, script)| (op, value, script)), + op => Ok((op, &[], script)), }, - _ => leading_byte, - }) + } } /** Encode/decode small integers: */ @@ -595,10 +547,12 @@ impl Script<'_> { let mut pc = self.0; let mut last_opcode = Opcode::Operation(OP_INVALIDOPCODE); while !pc.is_empty() { - let opcode = match Self::get_op(&mut pc) { + let (opcode, new_pc) = match Self::get_op(pc) { Ok(o) => o, + // Stop counting when we get to an invalid opcode. Err(_) => break, }; + pc = new_pc; if let Opcode::Operation(op) = opcode { if op == OP_CHECKSIG || op == OP_CHECKSIGVERIFY { n += 1; @@ -632,7 +586,8 @@ impl Script<'_> { pub fn is_push_only(&self) -> bool { let mut pc = self.0; while !pc.is_empty() { - if let Ok(Opcode::PushValue(_)) = Self::get_op(&mut pc) { + if let Ok((Opcode::PushValue(_), new_pc)) = Self::get_op(pc) { + pc = new_pc; } else { return false; } diff --git a/src/script_error.rs b/src/script_error.rs index 5c82bb72e..01219c0bd 100644 --- a/src/script_error.rs +++ b/src/script_error.rs @@ -1,6 +1,5 @@ #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum ScriptNumError { - NegativeZero, NonMinimalEncoding, Overflow { max_num_size: usize, actual: usize }, } @@ -44,7 +43,7 @@ pub enum ScriptError { SigDER, MinimalData, SigPushOnly, - SigHighS, // Unused (except in converting the C++ error to Rust) + SigHighS, SigNullDummy, PubKeyType, CleanStack,