fix(tariscript): protect compare and check height from underflows #5872

brianp · 2023-10-25T12:16:38Z

Description

This PR corrects the operand position of the handle_compare_height function. This was identified as incorrect based on the op code description.
Additionally, protect it from underflows, and write test cases against all possible returns and errors.

I added underflow protection in the handle_check_height function as well but after some thought this function wouldn't currently be possible to underflow. The values passed into the function would never be lower than 0, and then converted to i64 which would handle subtraction from 0 fine. In opposition, the handle_compare_height uses a value from the stack that could be i64::MIN and then have i64::MAX subtracted from it. Which was the originally identified problem. This is all to say the functions aren't quite equal so it felt worth a comment for future readers.

Motivation and Context

Closes #5813

How Has This Been Tested?

Added new tests.

What process can a PR reviewer use to test or verify this change?

Double check the docs to validate the operand switch is a valid change.

See the new underflow protection.

Breaking Changes

None
Requires data directory on base node to be deleted
Requires hard fork
Other - Please specify

github-actions · 2023-10-25T12:28:55Z

Test Results (CI)

1 253 tests 1 253 ✔️ 11m 32s ⏱️
    39 suites       0 💤
      1 files       0 ❌

Results for commit e1beaab.

♻️ This comment has been updated with latest results.

github-actions · 2023-10-25T12:35:06Z

Test Results (Integration tests)

31 tests 31 ✔️ 14m 34s ⏱️
11 suites   0 💤
  2 files   0 ❌

Results for commit e1beaab.

♻️ This comment has been updated with latest results.

SWvheerden

This looks good, just the error type

SWvheerden · 2023-10-26T08:08:53Z

infrastructure/tari_script/src/script.rs

+        // This differs from compare_height due to a stack number being used, which can be lower than 0
+        let item = match block_height.checked_sub(height) {
+            Some(num) => StackItem::Number(num),
+            None => return Err(ScriptError::CompareFailed),


Suggested change

None => return Err(ScriptError::CompareFailed),

None => return Err(ScriptError::StackUnderflow),

I would rather return the actual error here:

At one point I had the same thought but the StackUnderflow isn't a general Underflow error. I believe StackUnderflow is a specific error to the Input Stack underflowing during script execution. Which isn't the error here.

We could create an alternative error though. Change CompareFailed to Underflow or CompareFailed(message) with a specific message.

CompareFailed(message) is probably the best way forward. Compare failed lets you believe the comparison failed, not that its an underflow.

I suggest MathematicalUnderflow ^^ to differentaiate from StackUnderflow

Updated to CompareFailed(message)

SWvheerden · 2023-10-26T08:08:58Z

infrastructure/tari_script/src/script.rs

+        // height does not use a stack number and it's minimum can't be lower than 0.
+        let item = match target_height.checked_sub(block_height) {
+            Some(num) => StackItem::Number(num),
+            None => return Err(ScriptError::CompareFailed),


hansieodendaal

Hi @brianp, please see the comments below, especially the one in fn handle_compare_height.

hansieodendaal · 2023-11-01T12:13:39Z

infrastructure/tari_script/src/script.rs

-        let item = StackItem::Number(block_height - target_height);
+        // Here it is possible to underflow because the stack can take lower numbers where check
+        // height does not use a stack number and it's minimum can't be lower than 0.
+        let item = match target_height.checked_sub(block_height) {


I believe the order is the wrong way around here when compared to the original? Why did it change?

Suggested change

let item = match target_height.checked_sub(block_height) {

let item = match block_height.checked_sub(target_height) {

The previous behaviour is incorrect based on the documentation:

/// Pops the top of the stack as height, then pushes the value of (height - the current height) to the stack.
/// In other words, this opcode replaces the top of the stack with the difference between height and the
/// current height. Fails with InvalidInput if there is not a valid integer value on top of the stack. Fails
/// with StackUnderflow if the stack is empty.

So the question becomes which one is correct, the behaviour, or the documentation. I decided to trust the docs, and correct the behaviour. Do you have a particular reasoning for wanting to do the inverse?

Spoke with @SWvheerden who agrees with you on this @hansieodendaal. So I've reverted the operands to the original positions and updated the op codes docs to match this behaviour.

hansieodendaal · 2023-11-01T12:19:23Z

infrastructure/tari_script/src/error.rs

@@ -49,6 +49,8 @@ pub enum ScriptError {
    VerifyFailed,
    #[error("as_hash requires a Digest function that returns at least 32 bytes")]
    InvalidDigest,
+    #[error("A compare opcode failed, aborting the script immediately")]
+    CompareFailed,


Suggested change

CompareFailed,

MathematicalUnderflow,

hansieodendaal · 2023-11-01T12:19:40Z

infrastructure/tari_script/src/script.rs

+        // This differs from compare_height due to a stack number being used, which can be lower than 0
+        let item = match block_height.checked_sub(height) {
+            Some(num) => StackItem::Number(num),
+            None => return Err(ScriptError::CompareFailed),


I suggest MathematicalUnderflow ^^ to differentaiate from StackUnderflow

hansieodendaal · 2023-11-01T13:56:01Z

infrastructure/tari_script/src/script.rs

@@ -918,7 +930,7 @@ mod test {
            let ctx = context_with_height(u64::try_from(block_height).unwrap());
            assert_eq!(
                script.execute_with_context(&inputs, &ctx).unwrap(),
-                Number(block_height - 5)
+                Number(5 - block_height)


I would rather retain the original behaviour

The opscode docs define this function as the input height from the stack subtract the current block height. This was being done opposite which would result in reversed arithmetic. Also had underflow protection from the subtraction.

This check ins't needed due to the logic build up before its use but I've added it so we don't raise and false positives or convern in the future, and it protects against future changes. I also made not about the reasons it differs between its counterpart. And some test cases.

Update unseen existing test. The failure represents the exact change, and update will make it match new changes.

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 25, 2023

SWvheerden reviewed Oct 26, 2023

View reviewed changes

hansieodendaal reviewed Nov 1, 2023

View reviewed changes

brianp force-pushed the tari-script-underflows branch from 7bd4076 to d4d7175 Compare November 6, 2023 13:40

brianp added 6 commits November 6, 2023 16:06

Correct function to match documentation

65f41a5

The opscode docs define this function as the input height from the stack subtract the current block height. This was being done opposite which would result in reversed arithmetic. Also had underflow protection from the subtraction.

add test cases for compare height

5b5df5b

swappity swap

641fa89

Update unseen existing test. The failure represents the exact change, and update will make it match new changes.

Realign the positions and update the docs

92fca8f

Allow a message for the failure

e1beaab

brianp force-pushed the tari-script-underflows branch from d4d7175 to e1beaab Compare November 6, 2023 15:06

SWvheerden approved these changes Nov 7, 2023

View reviewed changes

ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 7, 2023

SWvheerden merged commit aa2ae10 into tari-project:development Nov 7, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tariscript): protect compare and check height from underflows #5872

fix(tariscript): protect compare and check height from underflows #5872

brianp commented Oct 25, 2023

github-actions bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

SWvheerden left a comment

SWvheerden Oct 26, 2023

brianp Oct 26, 2023

SWvheerden Oct 27, 2023

hansieodendaal Nov 1, 2023

brianp Nov 6, 2023

SWvheerden Oct 26, 2023

hansieodendaal left a comment

hansieodendaal Nov 1, 2023

brianp Nov 2, 2023

brianp Nov 6, 2023

hansieodendaal Nov 1, 2023

hansieodendaal Nov 1, 2023

hansieodendaal Nov 1, 2023

	None => return Err(ScriptError::CompareFailed),
	None => return Err(ScriptError::StackUnderflow),

	let item = match target_height.checked_sub(block_height) {
	let item = match block_height.checked_sub(target_height) {

fix(tariscript): protect compare and check height from underflows #5872

fix(tariscript): protect compare and check height from underflows #5872

Conversation

brianp commented Oct 25, 2023

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

github-actions bot commented Oct 25, 2023 • edited Loading

Test Results (CI)

github-actions bot commented Oct 25, 2023 • edited Loading

Test Results (Integration tests)

SWvheerden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading