From 7743f59020a609952a3dfb517f137e38eca2f4d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 20 Jul 2023 11:10:33 -0300 Subject: [PATCH] fix: return error when hint's PC is invalid (#1340) * Add test for failing program * Return error when some hint's PC isn't valid * Fix tests (+add test) * Appease clippy * Add some docs * Update changelog * Ignore manually compiled jsons in workflows cache * Fix warning in wasm-demo * Fix hash not hashing wasm-demo program * Fix mismatching paths --- .github/workflows/rust.yml | 48 ++++++++++++---- CHANGELOG.md | 2 + .../manually_compiled/invalid_hint_pc.json | 32 +++++++++++ .../manually_compiled/valid_program_a.json | 2 +- examples/wasm-demo/src/lib.rs | 1 - vm/src/serde/deserialize_program.rs | 56 +++++++++++++++++-- vm/src/tests/cairo_run_test.rs | 10 ++++ vm/src/types/errors/program_errors.rs | 2 + vm/src/types/program.rs | 27 ++++++--- vm/src/utils.rs | 4 +- 10 files changed, 157 insertions(+), 27 deletions(-) create mode 100644 cairo_programs/manually_compiled/invalid_hint_pc.json diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index d8f0ade90a..d7d8798613 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -31,6 +31,7 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json + !cairo_programs/manually_compiled/* examples/wasm-demo/src/array_sum.json key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} restore-keys: ${{ matrix.program-target }}-cache- @@ -82,6 +83,7 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json + !cairo_programs/manually_compiled/* examples/wasm-demo/src/array_sum.json key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch proof programs @@ -90,21 +92,27 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch bench programs uses: actions/cache/restore@v3 with: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_bench_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_bench_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch test contracts uses: actions/cache/restore@v3 with: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Run clippy run: make clippy @@ -139,6 +147,7 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json + !cairo_programs/manually_compiled/* examples/wasm-demo/src/array_sum.json key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch proof programs @@ -147,21 +156,27 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch bench programs uses: actions/cache/restore@v3 with: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_bench_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_bench_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch test contracts uses: actions/cache/restore@v3 with: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} # NOTE: we do this separately because --workspace operates in weird ways - name: Check all features (felt) @@ -215,6 +230,7 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json + !cairo_programs/manually_compiled/* examples/wasm-demo/src/array_sum.json key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch proof programs @@ -223,14 +239,18 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Fetch test contracts uses: actions/cache/restore@v3 with: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} - name: Install testing tools uses: taiki-e/install-action@v2 @@ -314,7 +334,7 @@ jobs: path: | cairo_programs/**/*.memory cairo_programs/**/*.trace - key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} restore-keys: ${{ matrix.program-target }}-reference-trace-cache- - name: Python3 Build @@ -334,7 +354,9 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: ${{ matrix.program-target }}-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} fail-on-cache-miss: true # This is not pretty, but we need `make` to see the compiled programs are @@ -377,7 +399,9 @@ jobs: path: | cairo_programs/**/*.casm cairo_programs/**/*.json - key: ${{ matrix.program-target }}-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + !cairo_programs/manually_compiled/* + examples/wasm-demo/src/array_sum.json + key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} fail-on-cache-miss: true - name: Generate traces @@ -467,7 +491,7 @@ jobs: path: | cairo_programs/**/*.memory cairo_programs/**/*.trace - key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }} + key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }} fail-on-cache-miss: true - name: Fetch traces for cairo-vm diff --git a/CHANGELOG.md b/CHANGELOG.md index 799dcfdea7..ceb3657bab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ #### Upcoming Changes +* fix: return error when a parsed hint's PC is invalid [#1340](https://github.com/lambdaclass/cairo-vm/pull/1340) + * chore(examples): remove _wee_alloc_ dependency from _wasm-demo_ example and _ensure-no_std_ dummy crate [#1337](https://github.com/lambdaclass/cairo-vm/pull/1337) * docs: improved crate documentation [#1334](https://github.com/lambdaclass/cairo-vm/pull/1334) diff --git a/cairo_programs/manually_compiled/invalid_hint_pc.json b/cairo_programs/manually_compiled/invalid_hint_pc.json new file mode 100644 index 0000000000..19cd2a8287 --- /dev/null +++ b/cairo_programs/manually_compiled/invalid_hint_pc.json @@ -0,0 +1,32 @@ +{ + "attributes": [], + "builtins": [], + "compiler_version": "0.11.0", + "data": [], + "debug_info": { + "instruction_locations": {} + }, + "hints": { + "18446744073709551615": [ + { + "accessible_scopes": [], + "code": "", + "flow_tracking_data": { + "ap_tracking": { + "group": 0, + "offset": 0 + }, + "reference_ids": {} + } + } + ] + }, + "identifiers": { + "__main__.main": {} + }, + "main_scope": "", + "prime": "0x800000000000011000000000000000000000000000000000000000000000001", + "reference_manager": { + "references": [] + } +} diff --git a/cairo_programs/manually_compiled/valid_program_a.json b/cairo_programs/manually_compiled/valid_program_a.json index dd9fd98240..02a48dc4c6 100644 --- a/cairo_programs/manually_compiled/valid_program_a.json +++ b/cairo_programs/manually_compiled/valid_program_a.json @@ -160,7 +160,7 @@ } } ], - "46": [ + "4": [ { "accessible_scopes": [ "__main__", diff --git a/examples/wasm-demo/src/lib.rs b/examples/wasm-demo/src/lib.rs index 37a65cc5c7..c3ad8d406c 100644 --- a/examples/wasm-demo/src/lib.rs +++ b/examples/wasm-demo/src/lib.rs @@ -12,7 +12,6 @@ extern "C" { fn log(msg: &str); } -#[cfg(feature = "console_error_panic_hook")] #[wasm_bindgen(start)] pub fn start() { crate::utils::set_panic_hook(); diff --git a/vm/src/serde/deserialize_program.rs b/vm/src/serde/deserialize_program.rs index 3506458571..e030493a1b 100644 --- a/vm/src/serde/deserialize_program.rs +++ b/vm/src/serde/deserialize_program.rs @@ -460,7 +460,8 @@ pub fn parse_program_json( } } - let (hints, hints_ranges) = Program::flatten_hints(&program_json.hints); + let (hints, hints_ranges) = + Program::flatten_hints(&program_json.hints, program_json.data.len())?; let shared_program_data = SharedProgramData { data: program_json.data, @@ -934,7 +935,7 @@ mod tests { }], ), ( - 46, + 4, vec![HintParams { code: "import math".to_string(), accessible_scopes: vec![ @@ -967,7 +968,7 @@ mod tests { /// Deserialize a program without an entrypoint. #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn deserialize_program_without_entrypoint_test() { + fn deserialize_program_without_entrypoint() { let reader = include_bytes!("../../../cairo_programs/manually_compiled/valid_program_a.json"); @@ -1003,7 +1004,7 @@ mod tests { }], ), ( - 46, + 4, vec![HintParams { code: "import math".to_string(), accessible_scopes: vec![ @@ -1584,4 +1585,51 @@ mod tests { .unwrap() ); } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn deserialize_program_with_invalid_hint_pc() { + let reader = br#"{ + "attributes": [], + "builtins": [], + "compiler_version": "0.11.0", + "data": [ + "0x41241" + ], + "debug_info": { + "instruction_locations": {} + }, + "hints": { + "1": [ + { + "accessible_scopes": [], + "code": "", + "flow_tracking_data": { + "ap_tracking": { + "group": 0, + "offset": 0 + }, + "reference_ids": {} + } + } + ] + }, + "identifiers": { + "__main__.main": {} + }, + "main_scope": "", + "prime": "0x800000000000011000000000000000000000000000000000000000000000001", + "reference_manager": { + "references": [] + } + }"#; + + let deserialization_result = deserialize_and_parse_program(reader, Some("main")); + + assert!(deserialization_result.is_err()); + assert_matches!( + deserialization_result.unwrap_err(), + ProgramError::InvalidHintPc(1, 1) + ); + } } diff --git a/vm/src/tests/cairo_run_test.rs b/vm/src/tests/cairo_run_test.rs index b6e2286e93..baa95b9f90 100644 --- a/vm/src/tests/cairo_run_test.rs +++ b/vm/src/tests/cairo_run_test.rs @@ -982,3 +982,13 @@ fn cairo_run_overflowing_dict() { include_bytes!("../../../cairo_programs/manually_compiled/overflowing_dict.json"); run_program_with_error(program_data, "Unknown memory cell at address"); } + +#[test] +fn cairo_run_big_hint_pcs() { + let program_data = + include_bytes!("../../../cairo_programs/manually_compiled/invalid_hint_pc.json"); + run_program_with_error( + program_data, + "Hint PC (18446744073709551615) is greater or equal to program length (0)", + ); +} diff --git a/vm/src/types/errors/program_errors.rs b/vm/src/types/errors/program_errors.rs index 7748874f17..4a3d16c712 100644 --- a/vm/src/types/errors/program_errors.rs +++ b/vm/src/types/errors/program_errors.rs @@ -17,6 +17,8 @@ pub enum ProgramError { ConstWithoutValue(String), #[error("Expected prime {PRIME_STR}, got {0}")] PrimeDiffers(String), + #[error("Hint PC ({0}) is greater or equal to program length ({1})")] + InvalidHintPc(usize, usize), } #[cfg(test)] diff --git a/vm/src/types/program.rs b/vm/src/types/program.rs index 03e21c5aba..033bbc2253 100644 --- a/vm/src/types/program.rs +++ b/vm/src/types/program.rs @@ -49,7 +49,8 @@ use arbitrary::Arbitrary; pub struct SharedProgramData { pub data: Vec, pub hints: Vec, - pub hints_ranges: Vec>, + /// This maps a PC to the range of hints in `hints` that correspond to it. + pub hints_ranges: Vec, pub main: Option, //start and end labels will only be used in proof-mode pub start: Option, @@ -60,6 +61,11 @@ pub struct SharedProgramData { pub reference_manager: Vec, } +/// Represents a range of hints corresponding to a PC. +/// +/// Is [`None`] if the range is empty, and it is [`Some`] tuple `(start, length)` otherwise. +type HintRange = Option<(usize, NonZeroUsize)>; + #[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(Arbitrary))] #[derive(Clone, Debug, PartialEq, Eq)] pub struct Program { @@ -91,7 +97,7 @@ impl Program { } } - let (hints, hints_ranges) = Self::flatten_hints(&hints); + let (hints, hints_ranges) = Self::flatten_hints(&hints, data.len())?; let shared_program_data = SharedProgramData { data, @@ -114,18 +120,23 @@ impl Program { pub(crate) fn flatten_hints( hints: &HashMap>, - ) -> (Vec, Vec>) { + program_length: usize, + ) -> Result<(Vec, Vec), ProgramError> { let bounds = hints .iter() .map(|(pc, hs)| (*pc, hs.len())) - .reduce(|(max_pc, full_len), (pc, len)| (max_pc.max(pc), full_len + len)); + .reduce(|(max_hint_pc, full_len), (pc, len)| (max_hint_pc.max(pc), full_len + len)); - let Some((max_pc, full_len)) = bounds else { - return (Vec::new(), Vec::new()); + let Some((max_hint_pc, full_len)) = bounds else { + return Ok((Vec::new(), Vec::new())); }; + if max_hint_pc >= program_length { + return Err(ProgramError::InvalidHintPc(max_hint_pc, program_length)); + } + let mut hints_values = Vec::with_capacity(full_len); - let mut hints_ranges = vec![None; max_pc + 1]; + let mut hints_ranges = vec![None; max_hint_pc + 1]; for (pc, hs) in hints.iter().filter(|(_, hs)| !hs.is_empty()) { let range = ( @@ -136,7 +147,7 @@ impl Program { hints_values.extend_from_slice(&hs[..]); } - (hints_values, hints_ranges) + Ok((hints_values, hints_ranges)) } #[cfg(feature = "std")] diff --git a/vm/src/utils.rs b/vm/src/utils.rs index f8fa9ed03d..7f8918dab2 100644 --- a/vm/src/utils.rs +++ b/vm/src/utils.rs @@ -343,7 +343,9 @@ pub mod test_utils { impl From for Program { fn from(val: ProgramFlat) -> Self { - let (hints, hints_ranges) = Program::flatten_hints(&val.hints); + // NOTE: panics if hints have PCs higher than the program length + let (hints, hints_ranges) = + Program::flatten_hints(&val.hints, val.data.len()).expect("hints are valid"); Program { shared_program_data: Arc::new(SharedProgramData { data: val.data,