Reject out-of-range OP_PICK and OP_ROLL indices without panicking

src/lib.rs

@@ -792,8 +792,10 @@ impl Exec {
             OP_PICK | OP_ROLL => {
                 // (xn ... x2 x1 x0 n - xn ... x2 x1 x0 xn)
                 // (xn ... x2 x1 x0 n - ... x2 x1 x0 xn)
+                self.stack.needn(2)?;
                 let x = self.stack.topnum(-1, self.opt.require_minimal)?;
-                if x < 0 || x >= self.stack.len() as i64 {
+                // The selector itself is not part of the selectable pool.
+                if x < 0 || x >= (self.stack.len() - 1) as i64 {
                     return Err(ExecError::InvalidStackOperation);
                 }
                 self.stack.pop().unwrap();

tests/stack_indices.rs

@@ -0,0 +1,126 @@
+//! Regression tests for the operand-stack bounds shared by OP_PICK and OP_ROLL.
+
+use bitcoin::{
+    absolute,
+    hashes::Hash,
+    opcodes::all::{OP_PICK, OP_ROLL},
+    transaction, Opcode, ScriptBuf, TapLeafHash, Transaction,
+};
+use bitcoin_scriptexec::{Exec, ExecCtx, ExecError, Options, TxTemplate};
+
+const CONTEXTS: [ExecCtx; 3] = [ExecCtx::Legacy, ExecCtx::SegwitV0, ExecCtx::Tapscript];
+
+fn executor(ctx: ExecCtx, opcode: Opcode, stack: Vec<Vec<u8>>) -> Exec {
+    Exec::new(
+        ctx,
+        Options::default(),
+        TxTemplate {
+            tx: Transaction {
+                version: transaction::Version::TWO,
+                lock_time: absolute::LockTime::ZERO,
+                input: vec![],
+                output: vec![],
+            },
+            prevouts: vec![],
+            input_idx: 0,
+            taproot_annex_scriptleaf: Some((TapLeafHash::all_zeros(), None)),
+        },
+        ScriptBuf::from_bytes(vec![opcode.to_u8()]),
+        stack,
+    )
+    .expect("single-opcode fixture")
+}
+
+fn assert_rejected(stack: Vec<Vec<u8>>, expected: ExecError) {
+    for ctx in CONTEXTS {
+        for opcode in [OP_PICK, OP_ROLL] {
+            let mut exec = executor(ctx, opcode, stack.clone());
+            // A panic is a test failure: invalid input must return a normal
+            // execution error, including when the index equals the pool size.
+            let result = exec.exec_next().expect_err("invalid stack index");
+            assert_eq!(result.error, Some(expected.clone()), "{ctx:?} {opcode:?}");
+            assert_eq!(result.opcode, Some(opcode));
+            assert!(!result.success);
+        }
+    }
+}
+
+#[test]
+fn first_and_last_stack_indices_are_valid() {
+    for ctx in CONTEXTS {
+        for (opcode, selector, expected) in [
+            (OP_PICK, 0, vec![0x11, 0x22, 0x33, 0x33]),
+            (OP_PICK, 2, vec![0x11, 0x22, 0x33, 0x11]),
+            (OP_ROLL, 0, vec![0x11, 0x22, 0x33]),
+            (OP_ROLL, 2, vec![0x22, 0x33, 0x11]),
+        ] {
+            let mut stack = vec![vec![0x11], vec![0x22], vec![0x33]];
+            stack.push(if selector == 0 {
+                vec![]
+            } else {
+                vec![selector]
+            });
+            let mut exec = executor(ctx, opcode, stack);
+            exec.exec_next().expect("valid stack index");
+            assert_eq!(
+                exec.stack().iter_str().collect::<Vec<_>>(),
+                expected
+                    .into_iter()
+                    .map(|value| vec![value])
+                    .collect::<Vec<_>>(),
+                "{ctx:?} {opcode:?} index {selector}",
+            );
+        }
+        for opcode in [OP_PICK, OP_ROLL] {
+            let mut exec = executor(ctx, opcode, vec![vec![0x42], vec![]]);
+            exec.exec_next().expect("only pool element is selectable");
+            let expected_len = if opcode == OP_PICK { 2 } else { 1 };
+            assert_eq!(
+                exec.stack().iter_str().collect::<Vec<_>>(),
+                vec![vec![0x42]; expected_len]
+            );
+        }
+    }
+}
+
+#[test]
+fn exact_pool_length_is_rejected_without_panicking() {
+    assert_rejected(vec![vec![0x42], vec![1]], ExecError::InvalidStackOperation);
+    assert_rejected(
+        vec![vec![0x11], vec![0x22], vec![0x33], vec![3]],
+        ExecError::InvalidStackOperation,
+    );
+}
+
+#[test]
+fn above_pool_length_is_rejected() {
+    assert_rejected(vec![vec![0x42], vec![2]], ExecError::InvalidStackOperation);
+    assert_rejected(
+        vec![vec![0x11], vec![0x22], vec![0x33], vec![4]],
+        ExecError::InvalidStackOperation,
+    );
+}
+
+#[test]
+fn negative_stack_index_is_rejected() {
+    assert_rejected(
+        vec![vec![0x42], vec![0x81]],
+        ExecError::InvalidStackOperation,
+    );
+}
+
+#[test]
+fn missing_operands_are_rejected_without_panicking() {
+    for stack in [vec![], vec![vec![]], vec![vec![0x42]], vec![vec![1; 5]]] {
+        assert_rejected(stack, ExecError::InvalidStackOperation);
+    }
+}
+
+#[test]
+fn malformed_stack_indices_keep_their_numeric_errors() {
+    assert_rejected(vec![vec![0x42], vec![0]], ExecError::MinimalData);
+    assert_rejected(
+        vec![vec![0x42], vec![1; 5]],
+        ExecError::ScriptIntNumericOverflow,
+    );
+}