#1482 Fix query fuzzing bugs

* Add query fuzzing tests * Fix assert that happens while reporting a query error * Fix assert when query has unbalanced {} scopes * Fix issue where query incorrectly treated this entity ref as variable
SanderMertens · Dec 21, 2024 · 963d8ab · 963d8ab
1 parent 6bf94e5
commit 963d8ab
Show file tree

Hide file tree

Showing 10 changed files with 290 additions and 9 deletions.
diff --git a/distr/flecs.c b/distr/flecs.c
@@ -34272,7 +34272,11 @@ void flecs_term_to_buf(
         {
             ecs_strbuf_appendlit(buf, "this");
         } else if (term->src.id & EcsIsVariable) {
-            ecs_strbuf_appendstr(buf, term->src.name);
+            if (term->src.name) {
+                ecs_strbuf_appendstr(buf, term->src.name);
+            } else {
+                ecs_strbuf_appendstr(buf, "<<invalid variable name>>");
+            }
         } else {
             /* Shouldn't happen */
         }
@@ -35843,6 +35847,7 @@ int flecs_query_finalize_terms(
 
         if (scope_nesting < 0) {
             flecs_query_validator_error(&ctx, "'}' without matching '{'");
+            return -1;
         }
     }
 
@@ -65559,7 +65564,12 @@ int flecs_query_discover_vars(
             table_this = true;
         }
 
-        if (ECS_TERM_REF_ID(first) == EcsThis || ECS_TERM_REF_ID(second) == EcsThis) {
+        bool first_is_this = 
+            (ECS_TERM_REF_ID(first) == EcsThis) && (first->id & EcsIsVariable);
+        bool second_is_this = 
+            (ECS_TERM_REF_ID(first) == EcsThis) && (first->id & EcsIsVariable);
+
+        if (first_is_this || second_is_this) {
             if (!table_this) {
                 entity_before_table_this = true;
             }
@@ -65761,6 +65771,7 @@ bool flecs_query_term_is_unknown(
     ecs_query_compile_ctx_t *ctx) 
 {
     ecs_query_op_t dummy = {0};
+
     flecs_query_compile_term_ref(NULL, query, &dummy, &term->first, 
         &dummy.first, EcsQueryFirst, EcsVarEntity, ctx, false);
     flecs_query_compile_term_ref(NULL, query, &dummy, &term->second, 
@@ -66177,6 +66188,7 @@ int flecs_query_compile(
     int32_t i, term_count = q->term_count;
     for (i = 0; i < term_count; i ++) {
         ecs_term_t *term = &terms[i];
+
         if (term->src.id & EcsIsEntity) {
             ecs_query_op_t set_fixed = {0};
             set_fixed.kind = EcsQuerySetFixed;
@@ -66421,6 +66433,8 @@ ecs_var_id_t flecs_query_find_var_id(
             if (query->pub.flags & EcsQueryHasTableThisVar) {
                 return 0;
             } else {
+                printf("VARNONE\n");
+                flecs_dump_backtrace(stdout);
                 return EcsVarNone;
             }
         }
@@ -67164,6 +67178,8 @@ static
 void flecs_query_compile_pop(
     ecs_query_compile_ctx_t *ctx)
 {
+    /* Should've been caught by query validator */
+    ecs_assert(ctx->scope > 0, ECS_INTERNAL_ERROR, NULL);
     ctx->cur = &ctx->ctrlflow[-- ctx->scope];
 }
 

diff --git a/src/query/compiler/compiler.c b/src/query/compiler/compiler.c
@@ -268,7 +268,12 @@ int flecs_query_discover_vars(
             table_this = true;
         }
 
-        if (ECS_TERM_REF_ID(first) == EcsThis || ECS_TERM_REF_ID(second) == EcsThis) {
+        bool first_is_this = 
+            (ECS_TERM_REF_ID(first) == EcsThis) && (first->id & EcsIsVariable);
+        bool second_is_this = 
+            (ECS_TERM_REF_ID(first) == EcsThis) && (first->id & EcsIsVariable);
+
+        if (first_is_this || second_is_this) {
             if (!table_this) {
                 entity_before_table_this = true;
             }
@@ -470,6 +475,7 @@ bool flecs_query_term_is_unknown(
     ecs_query_compile_ctx_t *ctx) 
 {
     ecs_query_op_t dummy = {0};
+
     flecs_query_compile_term_ref(NULL, query, &dummy, &term->first, 
         &dummy.first, EcsQueryFirst, EcsVarEntity, ctx, false);
     flecs_query_compile_term_ref(NULL, query, &dummy, &term->second, 
@@ -886,6 +892,7 @@ int flecs_query_compile(
     int32_t i, term_count = q->term_count;
     for (i = 0; i < term_count; i ++) {
         ecs_term_t *term = &terms[i];
+
         if (term->src.id & EcsIsEntity) {
             ecs_query_op_t set_fixed = {0};
             set_fixed.kind = EcsQuerySetFixed;

diff --git a/src/query/compiler/compiler_term.c b/src/query/compiler/compiler_term.c
@@ -19,6 +19,8 @@ ecs_var_id_t flecs_query_find_var_id(
             if (query->pub.flags & EcsQueryHasTableThisVar) {
                 return 0;
             } else {
+                printf("VARNONE\n");
+                flecs_dump_backtrace(stdout);
                 return EcsVarNone;
             }
         }
@@ -762,6 +764,8 @@ static
 void flecs_query_compile_pop(
     ecs_query_compile_ctx_t *ctx)
 {
+    /* Should've been caught by query validator */
+    ecs_assert(ctx->scope > 0, ECS_INTERNAL_ERROR, NULL);
     ctx->cur = &ctx->ctrlflow[-- ctx->scope];
 }
 

diff --git a/src/query/util.c b/src/query/util.c
@@ -556,7 +556,11 @@ void flecs_term_to_buf(
         {
             ecs_strbuf_appendlit(buf, "this");
         } else if (term->src.id & EcsIsVariable) {
-            ecs_strbuf_appendstr(buf, term->src.name);
+            if (term->src.name) {
+                ecs_strbuf_appendstr(buf, term->src.name);
+            } else {
+                ecs_strbuf_appendstr(buf, "<<invalid variable name>>");
+            }
         } else {
             /* Shouldn't happen */
         }

diff --git a/src/query/validator.c b/src/query/validator.c
@@ -1379,6 +1379,7 @@ int flecs_query_finalize_terms(
 
         if (scope_nesting < 0) {
             flecs_query_validator_error(&ctx, "'}' without matching '{'");
+            return -1;
         }
     }
 

diff --git a/test/query/project.json b/test/query/project.json
@@ -155,7 +155,8 @@
                 "validate_simple_w_transitive_pair",
                 "validate_simple_w_reflexive",
                 "validate_simple_w_reflexive_pair",
-                "validate_simple_w_inherited_component"
+                "validate_simple_w_inherited_component",
+                "validate_eq_this_not_a_var_w_wildcard"
             ]
         }, {
             "id": "Parser",
@@ -459,7 +460,24 @@
                 "escaped_identifier",
                 "escaped_identifier_first",
                 "escaped_identifier_second",
-                "n_tokens_test"
+                "n_tokens_test",
+                "this_not_a_var",
+                "eq_this_not_a_var",
+                "eq_this_not_a_var_w_wildcard"
+            ]
+        }, {
+            "id": "Fuzzing",
+            "setup": true,
+            "params": {
+                "cache_kind": ["default", "auto"]
+            },
+            "testcases": [
+                "1",
+                "2",
+                "3",
+                "4",
+                "5",
+                "6"
             ]
         }, {
             "id": "Basic",

diff --git a/test/query/src/Fuzzing.c b/test/query/src/Fuzzing.c
@@ -0,0 +1,61 @@
+#include <query.h>
+
+static ecs_query_cache_kind_t cache_kind = EcsQueryCacheDefault;
+
+void Fuzzing_setup(void) {
+    const char *cache_param = test_param("cache_kind");
+    if (cache_param) {
+        if (!strcmp(cache_param, "default")) {
+            // already set to default
+        } else if (!strcmp(cache_param, "auto")) {
+            cache_kind = EcsQueryCacheAuto;
+        } else {
+            printf("unexpected value for cache_param '%s'\n", cache_param);
+        }
+    }
+}
+
+static
+void fuzz(const char *expr) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_log_set_level(-4);
+
+    ecs_query_t *q = ecs_query(world, { 
+        .expr = expr,
+        .flags = EcsQueryAllowUnresolvedByName,
+        .cache_kind = cache_kind
+    });
+
+    if (q) {
+        ecs_query_fini(q);
+    }
+
+    test_assert(true);
+
+    ecs_fini(world);
+}
+
+void Fuzzing_1(void) {
+    fuzz("$is , *== $pe ||ee ||ethis  ");
+}
+
+void Fuzzing_2(void) {
+    fuzz("$is$, *==sKike}�}}}}(Docked}}________y__LikeXe $this  ");
+}
+
+void Fuzzing_3(void) {
+    fuzz("$$$m(), !_}, !{ ");
+}
+
+void Fuzzing_4(void) {
+    fuzz("$$}}#9}$i}}#9}$is , *== $this\\...........C..");
+}
+
+void Fuzzing_5(void) {
+    fuzz("$is == this,*");
+}
+
+void Fuzzing_6(void) {
+    fuzz("\x24\x6e\x23\x36\x2c\x23\x32\x2e\x2e\x2e\x2e\x2e\x2e\x24\x74\x6b\x6b\x69\x6b\x2e\x7d\x7d\x24\x6e\x24\x69\x2a\x2c\x20\x2a\x2c\x20\x2a\x2c\x2a\x2c\x2a\x2c\x2a\x2c\x24\x69\x2a\x2c\x20\x2a\x2c\x20\x2a\x7d\x7b\x7b\x2a\x2c\x20\x2a\x2c\x2a\x2c\x2a\x2c\x2a\x2c\x24\x69\x2a\x2c\x20\x2a\x2c\x20\x2a\x7d\x7b\x7b\x2a\x2c\x20\x2a\x2c\x2a\x2c\x2a\x2c");
+}
diff --git a/test/query/src/Parser.c b/test/query/src/Parser.c
@@ -7073,3 +7073,65 @@ void Parser_n_tokens_test(void) {
 
     ecs_fini(world);
 }
+
+void Parser_this_not_a_var(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_query_t *q = ecs_query(world, {
+        .expr = "this"
+    });
+
+    test_assert(q != NULL);
+
+    test_int(term_count(q), 1);
+
+    ecs_term_t *terms = query_terms(q);
+    test_first(terms[0], EcsThis, EcsSelf|EcsIsEntity);
+    test_src(terms[0], EcsThis, EcsSelf|EcsIsVariable);
+
+    ecs_query_fini(q);
+
+    ecs_fini(world);
+}
+
+void Parser_eq_this_not_a_var(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_query_t *q = ecs_query(world, {
+        .expr = "$foo == this"
+    });
+
+    test_assert(q != NULL);
+
+    test_int(term_count(q), 1);
+
+    ecs_term_t *terms = query_terms(q);
+    test_first(terms[0], EcsPredEq, EcsSelf|EcsIsEntity);
+    test_src_var(terms[0], 0, EcsSelf|EcsIsVariable, "foo");
+    test_second(terms[0], EcsThis, EcsSelf|EcsIsEntity);
+
+    ecs_query_fini(q);
+
+    ecs_fini(world);
+}
+
+void Parser_eq_this_not_a_var_w_wildcard(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_query_t *q = ecs_query(world, {
+        .expr = "$foo == this, *"
+    });
+
+    test_assert(q != NULL);
+
+    test_int(term_count(q), 2);
+
+    ecs_term_t *terms = query_terms(q);
+    test_first(terms[0], EcsPredEq, EcsSelf|EcsIsEntity);
+    test_src_var(terms[0], 0, EcsSelf|EcsIsVariable, "foo");
+    test_second(terms[0], EcsThis, EcsSelf|EcsIsEntity);
+
+    ecs_query_fini(q);
+
+    ecs_fini(world);
+}
diff --git a/test/query/src/Validator.c b/test/query/src/Validator.c
@@ -3598,3 +3598,42 @@ void Validator_validate_simple_w_inherited_component(void) {
 
     ecs_fini(world);
 }
+
+void Validator_validate_eq_this_not_a_var_w_wildcard(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_query_t *q = ecs_query(world, {
+        .terms = {
+            {
+                .first.id = EcsPredEq, 
+                .src.name = "$foo",
+                .second.name = "this"
+            },
+            { .id = EcsWildcard }
+        }
+    });
+
+    test_int(q->term_count, 2);
+    test_int(q->field_count, 2);
+    test_int(q->var_count, 2);
+    test_assert(q->vars != NULL);
+    test_assert(q->vars[0] == NULL);
+
+    test_uint(q->terms[0].id, ecs_pair(EcsPredEq, EcsThis));
+    test_int(q->terms[0].oper, EcsAnd);
+    test_int(q->terms[0].field_index, 0);
+    test_uint(q->terms[0].first.id, EcsPredEq|EcsSelf|EcsIsEntity);
+    test_uint(q->terms[0].src.id, EcsSelf|EcsIsVariable);
+    test_str(q->terms[0].src.name, "foo");
+    test_uint(q->terms[0].second.id, EcsThis|EcsSelf|EcsIsEntity);
+
+    test_uint(q->terms[1].id, EcsWildcard);
+    test_int(q->terms[1].oper, EcsAnd);
+    test_int(q->terms[1].field_index, 1);
+    test_uint(q->terms[1].first.id, EcsWildcard|EcsSelf|EcsIsVariable);
+    test_uint(q->terms[1].src.id, EcsThis|EcsSelf|EcsIsVariable);
+
+    ecs_query_fini(q);
+
+    ecs_fini(world);
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -1379,6 +1379,7 @@ int flecs_query_finalize_terms( @@
             if (scope_nesting < 0) {
                 flecs_query_validator_error(&ctx, "'}' without matching '{'");
+                return -1;
             }
         }
@@ Expand Down @@