Fix some compiler edge cases. (#1709)

* Fix some compiler edge cases.

Add a warning when "0 of them" is used. Point to some new documentation that
explains why "none of them" is preferred.

Handle some edge cases, which are all now errors:

* -1 of them
* "foo" of them
* /foo/ of them

And this one is also an error if the external variable "x" is a negative integer
or a string:

* x of them

I've also made sure this works with a construct like "0 + x of them" where x is
defined as a negative integer.

Add test cases for as many of these as I can. I don't have test cases for the
external variable case, but it does work as expected. Here is the output of a
rule using external variables where the variable is defined to be 1, 0, -1 and
foo:

```
wxs@mbp yara % cat rules/test.yara
rule a {
  strings:
    $a = "FreeBSD"
  condition:
    x of them
}
wxs@mbp yara % ./yara -d x=1 rules/test.yara /bin/ls
a /bin/ls
wxs@mbp yara % ./yara -d x=0 rules/test.yara /bin/ls
warning: rule "a" in rules/test.yara(5): Consider using "none" keyword, it is less ambiguous. Please see https://yara.readthedocs.io/en/stable/writingrules.html#sets-of-strings-1 for an explanation.
wxs@mbp yara % ./yara -d x=-1 rules/test.yara /bin/ls
error: rule "a" in rules/test.yara(5): invalid value in condition: "-1"
wxs@mbp yara % ./yara -d x=foo rules/test.yara /bin/ls
error: rule "a" in rules/test.yara(5): invalid value in condition: "string in for_expression is invalid"
wxs@mbp yara %
```

* More compiler warnings and errors.

If you specify "2 of ($a)" it is now a warning because it will always evaluate
to false, and is quite possibly not what you wanted to write. This is true for
string sets, rule sets and string sets in a range like "2 of ($a) in (0..10)".

If you are using an integer range it is now an error if the lower bound is
greater than the upper bound.

To support rule sets I needed to modify yr_parser_emit_pushes_for_rules() to
use a count parameter, which it will update with the number of pushed rules.
This is identical to how yr_parser_emit_pushes_for_strings() works.

While I'm here, remove the link from the warning message for the "none" keyword.

docs/writingrules.rst

@@ -1200,6 +1200,18 @@ The keywords ``any``, ``all`` and ``none`` can be used as well.
     1 of ($*)         // same that "any of them"
     none of ($b*)     // zero of the set of strings that start with "$b"
 
+.. warning:: Due to the way YARA works internally, using "0 of them" is an
+    ambiguous part of the language which should be avoided in favor of "none
+    of them". To understand this, consider the meaning of "2 of them", which
+    is true if 2 or more of the strings match. Historically, "0 of them"
+    followed this principle and would evaluate to true if at least one of the
+    strings matched. This ambiguity is resolved in YARA 4.3.0 by making "0 of
+    them" evaluate to true if exactly 0 of the strings match. To improve on
+    the situation and make the intent clear, it is encouraged to use "none" in
+    place of 0. By not using an integer it is easier to reason about the meaning
+    of "none of them" without the historical understanding of "at least 0"
+    clouding the issue.
+
 
 Starting with YARA 4.2.0 it is possible to express a set of strings in an
 integer range, like this:

libyara/compiler.c

@@ -1041,6 +1041,13 @@ YR_API char* yr_compiler_get_error_message(
         "rule identifier \"%s\" matches previously used wildcard rule set",
         compiler->last_error_extra_info);
     break;
+  case ERROR_INVALID_VALUE:
+    snprintf(
+        buffer,
+        buffer_size,
+        "invalid value in condition: \"%s\"",
+        compiler->last_error_extra_info);
+    break;
   }
 
   return buffer;

libyara/grammar.h

@@ -189,7 +189,7 @@ extern int yara_yydebug;
 #if ! defined YYSTYPE && ! defined YYSTYPE_IS_DECLARED
 union YYSTYPE
 {
-#line 336 "grammar.y"
+#line 341 "grammar.y"
 
   YR_EXPRESSION   expression;
   SIZED_STRING*   sized_string;

libyara/grammar.y

@@ -286,19 +286,24 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 %type <integer> integer_set
 %type <integer> integer_enumeration
-%type <integer> for_expression
 %type <integer> rule_modifier
 %type <integer> rule_modifiers
 %type <integer> string_enumeration
 %type <integer> string_enumeration_item
 %type <integer> string_set
 %type <integer> for_iteration
+%type <integer> rule_enumeration
+%type <integer> rule_enumeration_item
+%type <integer> rule_set
 
 %type <expression> primary_expression
 %type <expression> boolean_expression
 %type <expression> expression
 %type <expression> identifier
 %type <expression> regexp
+%type <expression> for_expression
+%type <expression> for_quantifier
+
 
 %type <c_string> arguments
 %type <c_string> arguments_list
@@ -1687,12 +1692,22 @@ expression
       }
     | for_expression _OF_ string_set
       {
+        if ($1.type == EXPRESSION_TYPE_INTEGER && $1.value.integer > $3)
+        {
+          yywarning(yyscanner,
+            "expression always false - requesting %lld of %lld.", $1.value.integer, $3);
+        }
         yr_parser_emit_with_arg(yyscanner, OP_OF, OF_STRING_SET, NULL, NULL);
 
         $$.type = EXPRESSION_TYPE_BOOLEAN;
       }
     | for_expression _OF_ rule_set
       {
+        if ($1.type == EXPRESSION_TYPE_INTEGER && $1.value.integer > $3)
+        {
+          yywarning(yyscanner,
+            "expression always false - requesting %lld of %lld.", $1.value.integer, $3);
+        }
         yr_parser_emit_with_arg(yyscanner, OP_OF, OF_RULE_SET, NULL, NULL);
 
         $$.type = EXPRESSION_TYPE_BOOLEAN;
@@ -1737,6 +1752,12 @@ expression
       }
     | for_expression _OF_ string_set _IN_ range
       {
+        if ($1.type == EXPRESSION_TYPE_INTEGER && $1.value.integer > $3)
+        {
+          yywarning(yyscanner,
+            "expression always false - requesting %lld of %lld.", $1.value.integer, $3);
+        }
+
         yr_parser_emit(yyscanner, OP_OF_FOUND_IN, NULL);
 
         $$.type = EXPRESSION_TYPE_BOOLEAN;
@@ -2106,6 +2127,18 @@ range
           result = ERROR_WRONG_TYPE;
         }
 
+        // If we can statically determine lower and upper bounds, ensure
+        // lower < upper. Check for upper bound here because some things (like
+        // string count) are EXPRESSION_TYPE_INTEGER.
+        if ($2.value.integer != YR_UNDEFINED &&
+            $4.value.integer != YR_UNDEFINED &&
+            $2.value.integer > $4.value.integer)
+        {
+          yr_compiler_set_error_extra_info(
+              compiler, "range lower bound must be greater than upper bound");
+          result = ERROR_INVALID_VALUE;
+        }
+
         fail_if_error(result);
       }
     ;
@@ -2214,12 +2247,15 @@ rule_set
         yr_parser_emit_push_const(yyscanner, YR_UNDEFINED);
       }
       rule_enumeration ')'
+      {
+        $$ = $3;
+      }
     ;
 
 
 rule_enumeration
-    : rule_enumeration_item
+    : rule_enumeration_item { $$ = $1; }
-    | rule_enumeration ',' rule_enumeration_item
+    | rule_enumeration ',' rule_enumeration_item { $$ = $1 + $3; }
     ;
 
 
@@ -2254,9 +2290,12 @@ rule_enumeration_item
         yr_free($1);
 
         fail_if_error(result);
+
+        $$ = 1;
       }
     | _IDENTIFIER_ '*'
       {
+        int count = 0;
         YR_NAMESPACE* ns = (YR_NAMESPACE*) yr_arena_get_ptr(
             compiler->arena,
             YR_NAMESPACES_TABLE,
@@ -2268,10 +2307,12 @@ rule_enumeration_item
             ns->name,
             1);
 
-        int result = yr_parser_emit_pushes_for_rules(yyscanner, $1);
+        int result = yr_parser_emit_pushes_for_rules(yyscanner, $1, &count);
         yr_free($1);
 
         fail_if_error(result);
+
+        $$ = count;
       }
     ;
 
@@ -2279,22 +2320,73 @@ rule_enumeration_item
 for_expression
     : primary_expression
       {
-        $$ = FOR_EXPRESSION_ANY;
+        if ($1.type == EXPRESSION_TYPE_INTEGER)
+        {
+          if ($1.value.integer == 0)
+          {
+            yywarning(yyscanner,
+                "consider using \"none\" keyword, it is less ambiguous.");
+          }
+
+          if ($1.value.integer < 0)
+          {
+            yr_compiler_set_error_extra_info_fmt(compiler,
+                "%lld", $1.value.integer);
+            fail_with_error(ERROR_INVALID_VALUE);
+          }
+        }
+
+        if ($1.type == EXPRESSION_TYPE_STRING)
+        {
+          SIZED_STRING* ss = yr_arena_ref_to_ptr(compiler->arena,
+              &$1.value.sized_string_ref);
+          // If the expression is an external string variable we need to get
+          // it some other way.
+          if (ss != NULL)
+          {
+            yr_compiler_set_error_extra_info_fmt(compiler, "%s", ss->c_string);
+          }
+          else
+          {
+            yr_compiler_set_error_extra_info(compiler,
+                "string in for_expression is invalid");
+          }
+          fail_with_error(ERROR_INVALID_VALUE);
+        }
+
+        if ($1.type == EXPRESSION_TYPE_REGEXP)
+        {
+          yr_compiler_set_error_extra_info(compiler,
+              "regexp in for_expression is invalid");
+          fail_with_error(ERROR_INVALID_VALUE);
+        }
+
+        $$.value.integer = $1.value.integer;
       }
-    | _ALL_
+    | for_quantifier
+      {
+        $$.value.integer = $1.value.integer;
+      }
+    ;
+
+for_quantifier
+    : _ALL_
       {
         yr_parser_emit_push_const(yyscanner, YR_UNDEFINED);
-        $$ = FOR_EXPRESSION_ALL;
+        $$.type = EXPRESSION_TYPE_QUANTIFIER;
-      }
+        $$.value.integer = FOR_EXPRESSION_ALL;
+     }
     | _ANY_
       {
         yr_parser_emit_push_const(yyscanner, 1);
-        $$ = FOR_EXPRESSION_ANY;
+        $$.type = EXPRESSION_TYPE_QUANTIFIER;
+        $$.value.integer = FOR_EXPRESSION_ANY;
       }
     | _NONE_
       {
         yr_parser_emit_push_const(yyscanner, 0);
-        $$ = FOR_EXPRESSION_NONE;
+        $$.type = EXPRESSION_TYPE_QUANTIFIER;
+        $$.value.integer = FOR_EXPRESSION_NONE;
       }
     ;
 
@@ -2468,7 +2560,7 @@ primary_expression
           {
             case OBJECT_TYPE_INTEGER:
               $$.type = EXPRESSION_TYPE_INTEGER;
-              $$.value.integer = YR_UNDEFINED;
+              $$.value.integer = $1.value.object->value.i;
               break;
             case OBJECT_TYPE_FLOAT:
               $$.type = EXPRESSION_TYPE_FLOAT;

libyara/include/yara/compiler.h

@@ -42,13 +42,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #define YARA_ERROR_LEVEL_WARNING 1
 
 // Expression type constants are powers of two because they are used as flags.
-#define EXPRESSION_TYPE_UNKNOWN 0
+#define EXPRESSION_TYPE_UNKNOWN    0
-#define EXPRESSION_TYPE_BOOLEAN 1
+#define EXPRESSION_TYPE_BOOLEAN    1
-#define EXPRESSION_TYPE_INTEGER 2
+#define EXPRESSION_TYPE_INTEGER    2
-#define EXPRESSION_TYPE_STRING  4
+#define EXPRESSION_TYPE_STRING     4
-#define EXPRESSION_TYPE_REGEXP  8
+#define EXPRESSION_TYPE_REGEXP     8
-#define EXPRESSION_TYPE_OBJECT  16
+#define EXPRESSION_TYPE_OBJECT     16
-#define EXPRESSION_TYPE_FLOAT   32
+#define EXPRESSION_TYPE_FLOAT      32
+#define EXPRESSION_TYPE_QUANTIFIER 64
 
 // The compiler uses an arena to store the data it generates during the
 // compilation. Each buffer in the arena is used for storing a different type

libyara/include/yara/error.h

@@ -106,6 +106,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #define ERROR_BLOCK_NOT_READY                61
 #define ERROR_INVALID_PERCENTAGE             62
 #define ERROR_IDENTIFIER_MATCHES_WILDCARD    63
+#define ERROR_INVALID_VALUE                  64
 
 #define GOTO_EXIT_ON_ERROR(x)    \
   {                              \

libyara/include/yara/parser.h

@@ -120,7 +120,8 @@ int yr_parser_emit_pushes_for_strings(
 
 int yr_parser_emit_pushes_for_rules(
     yyscan_t yyscanner,
-    const char* identifier);
+    const char* identifier,
+    int *count);
 
 int yr_parser_reduce_external(
     yyscan_t yyscanner,

libyara/parser.c

@@ -237,7 +237,10 @@ int yr_parser_emit_pushes_for_strings(
 
 // Emit OP_PUSH_RULE instructions for all rules whose identifier has given
 // prefix.
-int yr_parser_emit_pushes_for_rules(yyscan_t yyscanner, const char* prefix)
+int yr_parser_emit_pushes_for_rules(
+    yyscan_t yyscanner,
+    const char* prefix,
+    int* count)
 {
   YR_COMPILER* compiler = yyget_extra(yyscanner);
 
@@ -284,6 +287,11 @@ int yr_parser_emit_pushes_for_rules(yyscan_t yyscanner, const char* prefix)
     rule++;
   }
 
+  if (count != NULL)
+  {
+    *count = matching;
+  }
+
   if (matching == 0)
   {
     yr_compiler_set_error_extra_info(compiler, prefix);

tests/test-rules.c

@@ -477,6 +477,38 @@ static void test_syntax()
   // Test case for issue #1295
   assert_error("rule test rule test", ERROR_DUPLICATED_IDENTIFIER);
 
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: -1 of them }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: 0 + -1 of them }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: for -1 of them: ($) }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: for 0 + -1 of them: ($) }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: \"foo\" of them }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: for \"foo\" of them: ($) }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: /foo/ of them }",
+      ERROR_INVALID_VALUE);
+
+  assert_error(
+      "rule test { strings: $a = \"a\" condition: for /foo/ of them: ($) }",
+      ERROR_INVALID_VALUE);
+
   YR_DEBUG_FPRINTF(1, stderr, "} // %s()\n", __FUNCTION__);
 }
 
@@ -491,6 +523,93 @@ static void test_anonymous_strings()
   YR_DEBUG_FPRINTF(1, stderr, "} // %s()\n", __FUNCTION__);
 }
 
+static void test_warnings() {
+  YR_DEBUG_FPRINTF(1, stderr, "+ %s() {\n", __FUNCTION__);
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      0 of them \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      for 0 of ($a*): ($) \
+    }");
+
+  assert_no_warnings("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      none of them \
+    }");
+
+  assert_no_warnings("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      for none of ($a*): ($) \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      1 + -1 of them \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      2 of them \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      2 of ($a) \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      2 of ($a*) \
+    }");
+
+  assert_warning("rule test { \
+    strings: \
+      $a = \"AXSERS\" \
+    condition: \
+      2 of ($a*) in (0..10) \
+    }");
+
+  assert_warning("rule a { \
+    condition: \
+      true \
+    } \
+    rule b { \
+      condition: \
+        2 of (a) \
+    }");
+
+  assert_warning("rule a { \
+    condition: \
+      true \
+    } \
+    rule b { \
+      condition: \
+        2 of (a*) \
+    }");
+
+  YR_DEBUG_FPRINTF(1, stderr, "} // %s()\n", __FUNCTION__);
+}
+
 static void test_strings()
 {
   YR_DEBUG_FPRINTF(1, stderr, "+ %s() {\n", __FUNCTION__);
@@ -2045,6 +2164,24 @@ void test_for()
       }",
       NULL);
 
+  // Lower bound must be less than upper bound, if it can be determined statically.
+  assert_error(
+      "import \"tests\" \
+      rule test { \
+        condition: \
+          for any i in (10..1): (i) \
+      }",
+      ERROR_INVALID_VALUE);
+
+  // If one of the bounds can not be determined statically it isn't an error.
+  assert_true_rule(
+    "rule test { \
+      strings: \
+        $a = \"AXSERS\" \
+      condition: \
+        true or any of them in (0..filesize-100) \
+    }", TEXT_1024_BYTES);
+
   YR_DEBUG_FPRINTF(1, stderr, "} // %s()\n", __FUNCTION__);
 }
 
@@ -3537,6 +3674,7 @@ static void test_pass(int pass)
   test_global_rules();
   test_tags();
   test_meta();
+  test_warnings();
 
 #if !defined(USE_NO_PROC) && !defined(_WIN32) && !defined(__CYGWIN__)
   test_process_scan();