feat!: avoid integer overflows (#2713)

Co-authored-by: vezenovm <[email protected]> Co-authored-by: kevaundray <[email protected]> Co-authored-by: github-merge-queue[bot] <github-merge-queue[bot]@users.noreply.github.com> Co-authored-by: Tom French <[email protected]> Co-authored-by: kek kek kek <[email protected]> Co-authored-by: Martin Verzilli <[email protected]> Co-authored-by: Savio <[email protected]> Co-authored-by: Alex Gherghisan <[email protected]> Co-authored-by: jfecher <[email protected]> Co-authored-by: José Pedro Sousa <[email protected]> Co-authored-by: jfecher <[email protected]> Co-authored-by: Álvaro Rodríguez <[email protected]> Co-authored-by: josh crites <[email protected]> Co-authored-by: Gustavo Giráldez <[email protected]> Co-authored-by: Jan Beneš <[email protected]> Co-authored-by: Maddiaa <[email protected]> Co-authored-by: Tom French <[email protected]>
noir-lang · Oct 31, 2023 · 7d7d632 · 7d7d632
1 parent 3a4e6e6
commit 7d7d632
Show file tree

Hide file tree

Showing 9 changed files with 353 additions and 48 deletions.
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
@@ -18,6 +18,7 @@ use acvm::brillig_vm::brillig::HeapVector;
 use acvm::FieldElement;
 use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 use iter_extended::vecmap;
+use num_bigint::BigUint;
 
 use super::brillig_black_box::convert_black_box_call;
 use super::brillig_block_variables::BlockVariables;
@@ -554,6 +555,25 @@ impl<'block> BrilligBlock<'block> {
                     value_variable,
                 );
             }
+            Instruction::RangeCheck { value, max_bit_size, assert_message } => {
+                let left = self.convert_ssa_register_value(*value, dfg);
+                let max = BigUint::from(2_u128).pow(*max_bit_size);
+                let right = self.brillig_context.allocate_register();
+                self.brillig_context.const_instruction(
+                    right,
+                    FieldElement::from_be_bytes_reduce(&max.to_bytes_be()).into(),
+                );
+
+                let brillig_binary_op = BrilligBinaryOp::Integer {
+                    op: BinaryIntOp::LessThan,
+                    bit_size: max_bit_size + 1,
+                };
+                let condition = self.brillig_context.allocate_register();
+                self.brillig_context.binary_instruction(left, right, condition, brillig_binary_op);
+                self.brillig_context.constrain_instruction(condition, assert_message.clone());
+                self.brillig_context.deallocate_register(condition);
+                self.brillig_context.deallocate_register(right);
+            }
             _ => todo!("ICE: Instruction not supported {instruction:?}"),
         };
 

diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs
@@ -656,15 +656,23 @@ impl AcirContext {
         let remainder_var = r_value.into_var()?;
 
         // Constrain `q < 2^{max_q_bits}`.
-        self.range_constrain_var(quotient_var, &NumericType::Unsigned { bit_size: max_q_bits })?;
+        self.range_constrain_var(
+            quotient_var,
+            &NumericType::Unsigned { bit_size: max_q_bits },
+            None,
+        )?;
 
         // Constrain `r < 2^{max_rhs_bits}`.
         //
         // If `rhs` is a power of 2, then is just a looser version of the following bound constraint.
         // In the case where `rhs` isn't a power of 2 then this range constraint is required
         // as the bound constraint creates a new witness.
         // This opcode will be optimized out if it is redundant so we always add it for safety.
-        self.range_constrain_var(remainder_var, &NumericType::Unsigned { bit_size: max_rhs_bits })?;
+        self.range_constrain_var(
+            remainder_var,
+            &NumericType::Unsigned { bit_size: max_rhs_bits },
+            None,
+        )?;
 
         // Constrain `r < rhs`.
         self.bound_constraint_with_offset(remainder_var, rhs, predicate, max_rhs_bits)?;
@@ -768,12 +776,12 @@ impl AcirContext {
             let r_var = self.add_constant(r.into());
             let aor = self.add_var(lhs_offset, r_var)?;
             // lhs_offset<=rhs_offset <=> lhs_offset + r < rhs_offset + r = 2^bit_size <=> witness < 2^bit_size
-            self.range_constrain_var(aor, &NumericType::Unsigned { bit_size })?;
+            self.range_constrain_var(aor, &NumericType::Unsigned { bit_size }, None)?;
             return Ok(());
         }
         // General case:  lhs_offset<=rhs <=> rhs-lhs_offset>=0 <=> rhs-lhs_offset is a 'bits' bit integer
         let sub_expression = self.sub_var(rhs, lhs_offset)?; //rhs-lhs_offset
-        self.range_constrain_var(sub_expression, &NumericType::Unsigned { bit_size: bits })?;
+        self.range_constrain_var(sub_expression, &NumericType::Unsigned { bit_size: bits }, None)?;
 
         Ok(())
     }
@@ -874,6 +882,7 @@ impl AcirContext {
         &mut self,
         variable: AcirVar,
         numeric_type: &NumericType,
+        message: Option<String>,
     ) -> Result<AcirVar, RuntimeError> {
         match numeric_type {
             NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => {
@@ -886,6 +895,11 @@ impl AcirContext {
                 }
                 let witness = self.var_to_witness(variable)?;
                 self.acir_ir.range_constraint(witness, *bit_size)?;
+                if let Some(message) = message {
+                    self.acir_ir
+                        .assert_messages
+                        .insert(self.acir_ir.last_acir_opcode_location(), message);
+                }
             }
             NumericType::NativeField => {
                 // Range constraining a Field is a no-op

diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -347,7 +347,7 @@ impl Context {
     ) -> Result<AcirVar, RuntimeError> {
         let acir_var = self.acir_context.add_variable();
         if matches!(numeric_type, NumericType::Signed { .. } | NumericType::Unsigned { .. }) {
-            self.acir_context.range_constrain_var(acir_var, numeric_type)?;
+            self.acir_context.range_constrain_var(acir_var, numeric_type, None)?;
         }
         Ok(acir_var)
     }
@@ -529,6 +529,14 @@ impl Context {
             Instruction::Load { .. } => {
                 unreachable!("Expected all load instructions to be removed before acir_gen")
             }
+            Instruction::RangeCheck { value, max_bit_size, assert_message } => {
+                let acir_var = self.convert_numeric_value(*value, dfg)?;
+                self.acir_context.range_constrain_var(
+                    acir_var,
+                    &NumericType::Unsigned { bit_size: *max_bit_size },
+                    assert_message.clone(),
+                )?;
+            }
         }
         self.acir_context.set_call_stack(CallStack::new());
         Ok(())

diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
@@ -154,6 +154,9 @@ pub(crate) enum Instruction {
     /// Constrains two values to be equal to one another.
     Constrain(ValueId, ValueId, Option<String>),
 
+    /// Range constrain `value` to `max_bit_size`
+    RangeCheck { value: ValueId, max_bit_size: u32, assert_message: Option<String> },
+
     /// Performs a function call with a list of its arguments.
     Call { func: ValueId, arguments: Vec<ValueId> },
 
@@ -203,7 +206,8 @@ impl Instruction {
             Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array),
             Instruction::Constrain(..)
             | Instruction::Store { .. }
-            | Instruction::EnableSideEffects { .. } => InstructionResultType::None,
+            | Instruction::EnableSideEffects { .. }
+            | Instruction::RangeCheck { .. } => InstructionResultType::None,
             Instruction::Load { .. } | Instruction::ArrayGet { .. } | Instruction::Call { .. } => {
                 InstructionResultType::Unknown
             }
@@ -230,9 +234,12 @@ impl Instruction {
             Truncate { .. } => false,
 
             // These either have side-effects or interact with memory
-            Constrain(..) | EnableSideEffects { .. } | Allocate | Load { .. } | Store { .. } => {
-                false
-            }
+            Constrain(..)
+            | EnableSideEffects { .. }
+            | Allocate
+            | Load { .. }
+            | Store { .. }
+            | RangeCheck { .. } => false,
 
             Call { func, .. } => match dfg[*func] {
                 Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),
@@ -263,7 +270,7 @@ impl Instruction {
             | ArrayGet { .. }
             | ArraySet { .. } => false,
 
-            Constrain(..) | Store { .. } | EnableSideEffects { .. } => true,
+            Constrain(..) | Store { .. } | EnableSideEffects { .. } | RangeCheck { .. } => true,
 
             // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
             Call { func, .. } => match dfg[*func] {
@@ -320,6 +327,13 @@ impl Instruction {
             Instruction::ArraySet { array, index, value } => {
                 Instruction::ArraySet { array: f(*array), index: f(*index), value: f(*value) }
             }
+            Instruction::RangeCheck { value, max_bit_size, assert_message } => {
+                Instruction::RangeCheck {
+                    value: f(*value),
+                    max_bit_size: *max_bit_size,
+                    assert_message: assert_message.clone(),
+                }
+            }
         }
     }
 
@@ -364,6 +378,9 @@ impl Instruction {
             Instruction::EnableSideEffects { condition } => {
                 f(*condition);
             }
+            Instruction::RangeCheck { value, .. } => {
+                f(*value);
+            }
         }
     }
 
@@ -461,6 +478,14 @@ impl Instruction {
             Instruction::Allocate { .. } => None,
             Instruction::Load { .. } => None,
             Instruction::Store { .. } => None,
+            Instruction::RangeCheck { value, max_bit_size, .. } => {
+                if let Some(numeric_constant) = dfg.get_numeric_constant(*value) {
+                    if numeric_constant.num_bits() < *max_bit_size {
+                        return Remove;
+                    }
+                }
+                None
+            }
         }
     }
 }
@@ -484,7 +509,11 @@ fn simplify_cast(value: ValueId, dst_typ: &Type, dfg: &mut DataFlowGraph) -> Sim
                 SimplifiedTo(dfg.make_constant(constant, dst_typ.clone()))
             }
             (
-                Type::Numeric(NumericType::NativeField | NumericType::Unsigned { .. }),
+                Type::Numeric(
+                    NumericType::NativeField
+                    | NumericType::Unsigned { .. }
+                    | NumericType::Signed { .. },
+                ),
                 Type::Numeric(NumericType::Unsigned { bit_size }),
             ) => {
                 // Field/Unsigned -> unsigned: truncate
@@ -822,6 +851,29 @@ fn eval_constant_binary_op(
             let result = function(lhs, rhs);
             truncate(result, *bit_size).into()
         }
+        Type::Numeric(NumericType::Signed { bit_size }) => {
+            let function = operator.get_i128_function();
+
+            let lhs = truncate(lhs.try_into_u128()?, *bit_size);
+            let rhs = truncate(rhs.try_into_u128()?, *bit_size);
+            let l_pos = lhs < 2u128.pow(bit_size - 1);
+            let r_pos = rhs < 2u128.pow(bit_size - 1);
+            let lhs = if l_pos { lhs as i128 } else { -((2u128.pow(*bit_size) - lhs) as i128) };
+            let rhs = if r_pos { rhs as i128 } else { -((2u128.pow(*bit_size) - rhs) as i128) };
+            // The divisor is being truncated into the type of the operand, which can potentially
+            // lead to the rhs being zero.
+            // If the rhs of a division is zero, attempting to evaluate the division will cause a compiler panic.
+            // Thus, we do not evaluate the division in this method, as we want to avoid triggering a panic,
+            // and the operation should be handled by ACIR generation.
+            if matches!(operator, BinaryOp::Div | BinaryOp::Mod) && rhs == 0 {
+                return None;
+            }
+
+            let result = function(lhs, rhs);
+            let result =
+                if result >= 0 { result as u128 } else { (2i128.pow(*bit_size) + result) as u128 };
+            truncate(result, *bit_size).into()
+        }
         _ => return None,
     };
 
@@ -868,6 +920,21 @@ impl BinaryOp {
             BinaryOp::Lt => |x, y| (x < y) as u128,
         }
     }
+
+    fn get_i128_function(self) -> fn(i128, i128) -> i128 {
+        match self {
+            BinaryOp::Add => i128::wrapping_add,
+            BinaryOp::Sub => i128::wrapping_sub,
+            BinaryOp::Mul => i128::wrapping_mul,
+            BinaryOp::Div => i128::wrapping_div,
+            BinaryOp::Mod => i128::wrapping_rem,
+            BinaryOp::And => |x, y| x & y,
+            BinaryOp::Or => |x, y| x | y,
+            BinaryOp::Xor => |x, y| x ^ y,
+            BinaryOp::Eq => |x, y| (x == y) as i128,
+            BinaryOp::Lt => |x, y| (x < y) as i128,
+        }
+    }
 }
 
 /// Binary Operations allowed in the IR.

diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs
@@ -172,5 +172,8 @@ pub(crate) fn display_instruction(
                 show(*value)
             )
         }
+        Instruction::RangeCheck { value, max_bit_size, .. } => {
+            write!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)
+        }
     }
 }