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.

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!: check abi integer input is within signed range #7316

Merged
merged 23 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f41ec07
Check valid range when parsing abi strings into signed integers
asterite Feb 6, 2025
01e8b80
Also error on overflow when parsing integers
asterite Feb 6, 2025
8d5e57f
Better error messages
asterite Feb 6, 2025
4d25d93
Always use named arguments for input errors
asterite Feb 6, 2025
4862e65
Mention parameter names in parse errors, and show array index
asterite Feb 6, 2025
e5e98de
has an error -> is invalid
asterite Feb 6, 2025
745b35d
Try to fix JS tests
asterite Feb 6, 2025
6ed27d6
Fix tests
asterite Feb 6, 2025
e1fa7e6
Pass "-1" in test program using string
asterite Feb 6, 2025
37a0567
Use negative values in Prover.toml
asterite Feb 6, 2025
ac9c1f5
Allow negative integers in Prover.toml and Prover.json
asterite Feb 6, 2025
abd474c
Refactor
asterite Feb 6, 2025
befeb78
Continue disallowing negative integers in Prover.json
asterite Feb 6, 2025
7f22db7
No need to use BigInt in parse_integer_to_signed
asterite Feb 6, 2025
db21bc3
Also allow negative values (i64) in Prover.json
asterite Feb 7, 2025
31677ef
Update tooling/noirc_abi/src/errors.rs
asterite Feb 7, 2025
b6083c1
Update tooling/noirc_abi/src/input_parser/mod.rs
asterite Feb 7, 2025
5dd7ae9
Update tooling/noirc_abi/src/input_parser/mod.rs
asterite Feb 7, 2025
9f67cec
Update tooling/noirc_abi/src/input_parser/mod.rs
asterite Feb 7, 2025
f93d477
Fix tests
asterite Feb 7, 2025
96f2933
No need to have InputOutsideOfRange
asterite Feb 7, 2025
9517cec
Fix tests
asterite Feb 7, 2025
08ee034
Clearer error message names
asterite Feb 7, 2025
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
2 changes: 1 addition & 1 deletion test_programs/execution_success/signed_cmp/Prover.toml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
minus_one = 255
minus_one = -1
38 changes: 19 additions & 19 deletions test_programs/execution_success/signed_div/Prover.toml
Original file line number Diff line number Diff line change
@@ -1,51 +1,51 @@
[[ops]]
lhs = 4
rhs = 255 # -1
result = 252 # -4
rhs = -1
result = -4

[[ops]]
lhs = 4
rhs = 254 # -2
result = 254 # -2
rhs = -2
result = -2

[[ops]]
lhs = 4
rhs = 253 # -3
result = 255 # -1
rhs = -3
result = -1

[[ops]]
lhs = 4
rhs = 252 # -4
result = 255 # -1
rhs = -4
result = -1

[[ops]]
lhs = 4
rhs = 251 # -5
rhs = -5
result = 0

[[ops]]
lhs = 252 # -4
rhs = 255 # -1
lhs = -4
rhs = -1
result = 4

[[ops]]
lhs = 252 # -4
rhs = 254 # -2
lhs = -4
rhs = -2
result = 2

[[ops]]
lhs = 252 # -4
rhs = 253 # -3
lhs = -4
rhs = -3
result = 1

[[ops]]
lhs = 252 # -4
rhs = 252 # -4
lhs = -4
rhs = -4
result = 1

[[ops]]
lhs = 252 # -4
rhs = 251 # -5
lhs = -4
rhs = -5
result = 0


Expand Down
2 changes: 1 addition & 1 deletion tooling/noir_js/test/node/cjs.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ describe('input validation', () => {
chai
.expect(knownError.message)
.to.equal(
'Expected witness values to be integers, provided value causes `invalid digit found in string` error',
'The value passed for parameter `x` is invalid:\nExpected witness values to be integers, provided value causes `invalid digit found in string` error',
);
}
});
Expand Down
2 changes: 1 addition & 1 deletion tooling/noir_js/test/node/smoke.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('input validation', () => {
} catch (error) {
const knownError = error as Error;
expect(knownError.message).to.equal(
'Expected witness values to be integers, provided value causes `invalid digit found in string` error',
'The value passed for parameter `x` is invalid:\nExpected witness values to be integers, provided value causes `invalid digit found in string` error',
);
}
});
Expand Down
24 changes: 19 additions & 5 deletions tooling/noirc_abi/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,31 @@ use crate::{
input_parser::{InputTypecheckingError, InputValue},
AbiType,
};
use acvm::acir::native_types::Witness;
use acvm::{acir::native_types::Witness, AcirField, FieldElement};
use num_bigint::BigInt;
use thiserror::Error;

#[derive(Debug, Error)]
pub enum InputParserError {
#[error("input file is badly formed, could not parse, {0}")]
ParseInputMap(String),
#[error("Expected witness values to be integers, provided value causes `{0}` error")]
ParseStr(String),
#[error("Could not parse hex value {0}")]
ParseHexStr(String),
#[error(
"The value passed for parameter `{arg_name}` is invalid:\nExpected witness values to be integers, provided value causes `{value}` error"
asterite marked this conversation as resolved.
Show resolved Hide resolved
)]
ParseStr { arg_name: String, value: String },
#[error("The value passed for parameter `{arg_name}` is invalid:\nValue {value} is less than minimum allowed value of {min}")]
InputExceedsMinimum { arg_name: String, value: String, min: String },
#[error("The value passed for parameter `{arg_name}` is invalid:\nValue {value} exceeds maximum allowed value of {max}")]
InputExceedsMaximum { arg_name: String, value: String, max: String },
#[error(
"The value passed for parameter `{arg_name}` is invalid:\nValue {value} outside of valid range. Value must fall within [{min}, {max}]"
)]
InputOutsideOfRange { arg_name: String, value: BigInt, min: BigInt, max: BigInt },
asterite marked this conversation as resolved.
Show resolved Hide resolved
#[error(
"The value passed for parameter `{arg_name}` is invalid:\nValue {value} exceeds field modulus. Values must fall within [0, {})",
FieldElement::modulus()
)]
InputExceedsFieldModulus { arg_name: String, value: String },
#[error("cannot parse value into {0:?}")]
AbiTypeMismatch(AbiType),
#[error("Expected argument `{0}`, but none was found")]
Expand Down
44 changes: 38 additions & 6 deletions tooling/noirc_abi/src/input_parser/json.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use super::{field_to_signed_hex, parse_str_to_field, parse_str_to_signed, InputValue};
use super::{
field_to_signed_hex, parse_integer_to_signed, parse_str_to_field, parse_str_to_signed,
InputValue,
};
use crate::{errors::InputParserError, Abi, AbiType, MAIN_RETURN_NAME};
use acvm::{AcirField, FieldElement};
use iter_extended::{try_btree_map, try_vecmap};
Expand Down Expand Up @@ -147,12 +150,20 @@ impl InputValue {
let input_value = match (value, param_type) {
(JsonTypes::String(string), AbiType::String { .. }) => InputValue::String(string),
(JsonTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => {
InputValue::Field(parse_str_to_signed(&string, *width)?)
InputValue::Field(parse_str_to_signed(&string, *width, arg_name)?)
}
(
JsonTypes::String(string),
AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean,
) => InputValue::Field(parse_str_to_field(&string)?),
) => InputValue::Field(parse_str_to_field(&string, arg_name)?),

(
JsonTypes::Integer(integer),
AbiType::Integer { sign: crate::Sign::Signed, width },
) => {
let new_value = parse_integer_to_signed(integer as i128, *width, arg_name)?;
InputValue::Field(new_value)
}

(
JsonTypes::Integer(integer),
Expand All @@ -166,8 +177,13 @@ impl InputValue {
(JsonTypes::Bool(boolean), AbiType::Boolean) => InputValue::Field(boolean.into()),

(JsonTypes::Array(array), AbiType::Array { typ, .. }) => {
let array_elements =
try_vecmap(array, |value| InputValue::try_from_json(value, typ, arg_name))?;
let mut index = 0;
let array_elements = try_vecmap(array, |value| {
let sub_name = format!("{arg_name}[{index}]");
let value = InputValue::try_from_json(value, typ, &sub_name);
index += 1;
value
})?;
InputValue::Vec(array_elements)
}

Expand All @@ -186,8 +202,12 @@ impl InputValue {
}

(JsonTypes::Array(array), AbiType::Tuple { fields }) => {
let mut index = 0;
let tuple_fields = try_vecmap(array.into_iter().zip(fields), |(value, typ)| {
InputValue::try_from_json(value, typ, arg_name)
let sub_name = format!("{arg_name}[{index}]");
let value = InputValue::try_from_json(value, typ, &sub_name);
index += 1;
value
})?;
InputValue::Vec(tuple_fields)
}
Expand All @@ -206,6 +226,7 @@ mod test {
use crate::{
arbitrary::arb_abi_and_input_map,
input_parser::{arbitrary::arb_signed_integer_type_and_value, json::JsonTypes, InputValue},
AbiType,
};

use super::{parse_json, serialize_to_json};
Expand Down Expand Up @@ -234,4 +255,15 @@ mod test {
prop_assert_eq!(output_number, value);
}
}

#[test]
fn errors_on_integer_to_signed_integer_overflow() {
let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 8 };
let input = JsonTypes::Integer(128);
assert!(InputValue::try_from_json(input, &typ, "foo").is_err());

let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 16 };
let input = JsonTypes::Integer(32768);
assert!(InputValue::try_from_json(input, &typ, "foo").is_err());
}
}
Loading
Loading