From 1dd3ade17cb499f19f6261e0e8cecebb56e6de9a Mon Sep 17 00:00:00 2001 From: "Victor M. Alvarez" Date: Fri, 2 Aug 2024 13:40:56 +0200 Subject: [PATCH] feat: accept comparison between boolean expression and integer constant Expressions like `pe.is_signed == 1` are now accepted. This kind of expressions are valid in legacy YARA, and very common. So we are now accepting them, but raising a warning. --- lib/src/compiler/ir/ast2ir.rs | 47 ++++++++++++++++++- lib/src/compiler/report.rs | 14 ++++++ .../compiler/tests/testdata/warnings/27.in | 5 ++ .../compiler/tests/testdata/warnings/27.out | 6 +++ .../compiler/tests/testdata/warnings/28.in | 5 ++ .../compiler/tests/testdata/warnings/28.out | 6 +++ .../compiler/tests/testdata/warnings/29.in | 5 ++ .../compiler/tests/testdata/warnings/29.out | 6 +++ .../compiler/tests/testdata/warnings/30.in | 5 ++ .../compiler/tests/testdata/warnings/30.out | 6 +++ lib/src/compiler/warnings.rs | 8 ++++ lib/src/modules/test_proto2/tests/mod.rs | 5 ++ 12 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 lib/src/compiler/tests/testdata/warnings/27.in create mode 100644 lib/src/compiler/tests/testdata/warnings/27.out create mode 100644 lib/src/compiler/tests/testdata/warnings/28.in create mode 100644 lib/src/compiler/tests/testdata/warnings/28.out create mode 100644 lib/src/compiler/tests/testdata/warnings/29.in create mode 100644 lib/src/compiler/tests/testdata/warnings/29.out create mode 100644 lib/src/compiler/tests/testdata/warnings/30.in create mode 100644 lib/src/compiler/tests/testdata/warnings/30.out diff --git a/lib/src/compiler/ir/ast2ir.rs b/lib/src/compiler/ir/ast2ir.rs index d3b7ca6d7..4c0c564d1 100644 --- a/lib/src/compiler/ir/ast2ir.rs +++ b/lib/src/compiler/ir/ast2ir.rs @@ -438,7 +438,52 @@ pub(in crate::compiler) fn expr_from_ast( ast::Expr::BitwiseXor(expr) => bitwise_xor_expr_from_ast(ctx, expr), // Comparison operations - ast::Expr::Eq(expr) => eq_expr_from_ast(ctx, expr), + ast::Expr::Eq(expr) => { + let span = expr.span(); + + let lhs_span = expr.lhs.span(); + let rhs_span = expr.rhs.span(); + + let lhs = expr_from_ast(ctx, &expr.lhs)?; + let rhs = expr_from_ast(ctx, &expr.rhs)?; + + // Detect cases in which the equal operator is comparing a boolean + // expression with an integer constant (e.g: `pe.is_signed == 0`). + // This is quite common in YARA rules, it is accepted without + // errors, but a warning is raised. + let replacement = match (lhs.type_value(), rhs.type_value()) { + (TypeValue::Bool(_), TypeValue::Integer(Value::Const(0))) => { + Some(( + Expr::Not { operand: Box::new(lhs) }, + format!("not {}", ctx.report_builder.get_snippet(&lhs_span.into())) + )) + } + (TypeValue::Integer(Value::Const(0)), TypeValue::Bool(_)) => { + Some(( + Expr::Not { operand: Box::new(rhs) }, + format!("not {}", ctx.report_builder.get_snippet(&rhs_span.into())) + )) + } + (TypeValue::Bool(_), TypeValue::Integer(Value::Const(1))) => { + Some((lhs, ctx.report_builder.get_snippet(&lhs_span.into()))) + } + (TypeValue::Integer(Value::Const(1)), TypeValue::Bool(_)) => { + Some((rhs, ctx.report_builder.get_snippet(&rhs_span.into()))) + } + _ => None + }; + + if let Some((expr, msg)) = replacement { + ctx.warnings.add(|| Warning::boolean_integer_comparison( + ctx.report_builder, + span.into(), + msg, + )); + Ok(expr) + } else { + eq_expr_from_ast(ctx, expr) + } + }, ast::Expr::Ne(expr) => ne_expr_from_ast(ctx, expr), ast::Expr::Gt(expr) => gt_expr_from_ast(ctx, expr), ast::Expr::Ge(expr) => ge_expr_from_ast(ctx, expr), diff --git a/lib/src/compiler/report.rs b/lib/src/compiler/report.rs index 543b29c6b..7d1c7817a 100644 --- a/lib/src/compiler/report.rs +++ b/lib/src/compiler/report.rs @@ -137,6 +137,20 @@ impl ReportBuilder { self } + /// Returns the fragment of source code indicated by `source_ref`. + pub fn get_snippet(&self, source_ref: &SourceRef) -> String { + let source_id = source_ref + .source_id + .or_else(|| self.current_source_id()) + .expect("create_report without registering any source code"); + + let cache = self.cache.borrow(); + let cache_entry = cache.data.get(&source_id).unwrap(); + let src = cache_entry.code.as_str(); + + src[source_ref.span.range()].to_string() + } + /// Creates a new error or warning report. pub fn create_report( &self, diff --git a/lib/src/compiler/tests/testdata/warnings/27.in b/lib/src/compiler/tests/testdata/warnings/27.in new file mode 100644 index 000000000..fe356765d --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/27.in @@ -0,0 +1,5 @@ +import "test_proto2" + +rule test { + condition: test_proto2.array_bool[0] == 1 +} \ No newline at end of file diff --git a/lib/src/compiler/tests/testdata/warnings/27.out b/lib/src/compiler/tests/testdata/warnings/27.out new file mode 100644 index 000000000..a6e58ff6b --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/27.out @@ -0,0 +1,6 @@ +warning[bool_int_comparison]: comparison between boolean and integer + --> line:4:13 + | +4 | condition: test_proto2.array_bool[0] == 1 + | ------------------------------ this comparison can be replaced with: `test_proto2.array_bool[0]` + | diff --git a/lib/src/compiler/tests/testdata/warnings/28.in b/lib/src/compiler/tests/testdata/warnings/28.in new file mode 100644 index 000000000..94393e446 --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/28.in @@ -0,0 +1,5 @@ +import "test_proto2" + +rule test { + condition: test_proto2.array_bool[0] == 0 +} \ No newline at end of file diff --git a/lib/src/compiler/tests/testdata/warnings/28.out b/lib/src/compiler/tests/testdata/warnings/28.out new file mode 100644 index 000000000..420e9d7b8 --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/28.out @@ -0,0 +1,6 @@ +warning[bool_int_comparison]: comparison between boolean and integer + --> line:4:13 + | +4 | condition: test_proto2.array_bool[0] == 0 + | ------------------------------ this comparison can be replaced with: `not test_proto2.array_bool[0]` + | diff --git a/lib/src/compiler/tests/testdata/warnings/29.in b/lib/src/compiler/tests/testdata/warnings/29.in new file mode 100644 index 000000000..a3ce69538 --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/29.in @@ -0,0 +1,5 @@ +import "test_proto2" + +rule test { + condition: 1 == test_proto2.array_bool[0] +} \ No newline at end of file diff --git a/lib/src/compiler/tests/testdata/warnings/29.out b/lib/src/compiler/tests/testdata/warnings/29.out new file mode 100644 index 000000000..0cd3b45eb --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/29.out @@ -0,0 +1,6 @@ +warning[bool_int_comparison]: comparison between boolean and integer + --> line:4:13 + | +4 | condition: 1 == test_proto2.array_bool[0] + | ------------------------------ this comparison can be replaced with: `test_proto2.array_bool[0]` + | diff --git a/lib/src/compiler/tests/testdata/warnings/30.in b/lib/src/compiler/tests/testdata/warnings/30.in new file mode 100644 index 000000000..68793c87c --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/30.in @@ -0,0 +1,5 @@ +import "test_proto2" + +rule test { + condition: 0 == test_proto2.array_bool[0] +} \ No newline at end of file diff --git a/lib/src/compiler/tests/testdata/warnings/30.out b/lib/src/compiler/tests/testdata/warnings/30.out new file mode 100644 index 000000000..c2cf46b3f --- /dev/null +++ b/lib/src/compiler/tests/testdata/warnings/30.out @@ -0,0 +1,6 @@ +warning[bool_int_comparison]: comparison between boolean and integer + --> line:4:13 + | +4 | condition: 0 == test_proto2.array_bool[0] + | ------------------------------ this comparison can be replaced with: `not test_proto2.array_bool[0]` + | diff --git a/lib/src/compiler/warnings.rs b/lib/src/compiler/warnings.rs index 1c6b43ff5..5a0986845 100644 --- a/lib/src/compiler/warnings.rs +++ b/lib/src/compiler/warnings.rs @@ -51,6 +51,14 @@ pub enum Warning { note: Option, }, + #[warning("bool_int_comparison", "comparison between boolean and integer")] + #[label("this comparison can be replaced with: `{replacement}`", span)] + BooleanIntegerComparison { + detailed_report: String, + span: SourceRef, + replacement: String, + }, + #[warning("duplicate_import", "duplicate import statement")] #[label( "duplicate import", diff --git a/lib/src/modules/test_proto2/tests/mod.rs b/lib/src/modules/test_proto2/tests/mod.rs index acc3b1ccb..5f1737fcb 100644 --- a/lib/src/modules/test_proto2/tests/mod.rs +++ b/lib/src/modules/test_proto2/tests/mod.rs @@ -75,6 +75,11 @@ fn test_proto2_module() { condition_false!(r#"test_proto2.array_bool[0]"#); condition_true!(r#"test_proto2.array_bool[1]"#); + condition_false!(r#"test_proto2.array_bool[0] == 1"#); + condition_true!(r#"test_proto2.array_bool[0] == 0"#); + condition_false!(r#"1 == test_proto2.array_bool[0]"#); + condition_true!(r#"0 == test_proto2.array_bool[0]"#); + // array_int64[3] is undefined, so both conditions are false. condition_false!(r#"test_proto2.array_int64[3] == 0"#); condition_false!(r#"test_proto2.array_int64[3] != 0"#);