Fix issue with setting NULL as doc string, crash in ecs_iter_str

SanderMertens · Oct 7, 2023 · a96da13 · a96da13
1 parent fbb237f
commit a96da13
Show file tree

Hide file tree

Showing 12 changed files with 251 additions and 51 deletions.
diff --git a/flecs.c b/flecs.c
@@ -24689,59 +24689,61 @@ static ECS_DTOR(EcsDocDescription, ptr, {
     ecs_os_free((char*)ptr->value);
 })
 
+static
+void flecs_doc_set(
+    ecs_world_t *world,
+    ecs_entity_t entity,
+    ecs_entity_t kind,
+    const char *value)
+{
+    if (value) {
+        ecs_set_pair(world, entity, EcsDocDescription, kind, {
+            /* Safe, value gets copied by copy hook */
+            .value = ECS_CONST_CAST(char*, value)
+        });
+    } else {
+        ecs_remove_pair(world, entity, ecs_id(EcsDocDescription), kind);
+    }
+}
+
 void ecs_doc_set_name(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *name)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsName, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, name)
-    });
+    flecs_doc_set(world, entity, EcsName, name);
 }
 
 void ecs_doc_set_brief(
     ecs_world_t *world,
     ecs_entity_t entity,
-    const char *description)
+    const char *brief)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocBrief, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, description)
-    });
+    flecs_doc_set(world, entity, EcsDocBrief, brief);
 }
 
 void ecs_doc_set_detail(
     ecs_world_t *world,
     ecs_entity_t entity,
-    const char *description)
+    const char *detail)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocDetail, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, description)
-    });
+    flecs_doc_set(world, entity, EcsDocDetail, detail);
 }
 
 void ecs_doc_set_link(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *link)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocLink, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, link)
-    });
+    flecs_doc_set(world, entity, EcsDocLink, link);
 }
 
 void ecs_doc_set_color(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *color)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocColor, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, color)
-    });
+    flecs_doc_set(world, entity, EcsDocColor, color);
 }
 
 const char* ecs_doc_get_name(

diff --git a/src/addons/doc.c b/src/addons/doc.c
@@ -22,59 +22,61 @@ static ECS_DTOR(EcsDocDescription, ptr, {
     ecs_os_free((char*)ptr->value);
 })
 
+static
+void flecs_doc_set(
+    ecs_world_t *world,
+    ecs_entity_t entity,
+    ecs_entity_t kind,
+    const char *value)
+{
+    if (value) {
+        ecs_set_pair(world, entity, EcsDocDescription, kind, {
+            /* Safe, value gets copied by copy hook */
+            .value = ECS_CONST_CAST(char*, value)
+        });
+    } else {
+        ecs_remove_pair(world, entity, ecs_id(EcsDocDescription), kind);
+    }
+}
+
 void ecs_doc_set_name(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *name)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsName, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, name)
-    });
+    flecs_doc_set(world, entity, EcsName, name);
 }
 
 void ecs_doc_set_brief(
     ecs_world_t *world,
     ecs_entity_t entity,
-    const char *description)
+    const char *brief)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocBrief, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, description)
-    });
+    flecs_doc_set(world, entity, EcsDocBrief, brief);
 }
 
 void ecs_doc_set_detail(
     ecs_world_t *world,
     ecs_entity_t entity,
-    const char *description)
+    const char *detail)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocDetail, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, description)
-    });
+    flecs_doc_set(world, entity, EcsDocDetail, detail);
 }
 
 void ecs_doc_set_link(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *link)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocLink, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, link)
-    });
+    flecs_doc_set(world, entity, EcsDocLink, link);
 }
 
 void ecs_doc_set_color(
     ecs_world_t *world,
     ecs_entity_t entity,
     const char *color)
 {
-    ecs_set_pair(world, entity, EcsDocDescription, EcsDocColor, {
-        /* Safe, value gets copied by copy hook */
-        .value = ECS_CONST_CAST(char*, color)
-    });
+    flecs_doc_set(world, entity, EcsDocColor, color);
 }
 
 const char* ecs_doc_get_name(

diff --git a/src/iter.c b/src/iter.c
@@ -474,6 +474,10 @@ size_t ecs_field_size(
 char* ecs_iter_str(
     const ecs_iter_t *it)
 {
+    if (!(it->flags & EcsIterIsValid)) {
+        return NULL;
+    }
+
     ecs_world_t *world = it->world;
     ecs_strbuf_t buf = ECS_STRBUF_INIT;
     int i;
@@ -508,7 +512,7 @@ char* ecs_iter_str(
         ecs_strbuf_list_pop(&buf, "\n");
     }
 
-    if (it->variable_count) {
+    if (it->variable_count && it->variable_names) {
         int32_t actual_count = 0;
         for (i = 0; i < it->variable_count; i ++) {
             const char *var_name = it->variable_names[i];

diff --git a/test/addons/project.json b/test/addons/project.json
@@ -483,7 +483,12 @@
                 "get_entity_name",
                 "get_set_brief",
                 "get_set_detail",
-                "get_set_link"
+                "get_set_link",
+                "set_name_nullptr",
+                "set_brief_nullptr",
+                "set_detail_nullptr",
+                "set_link_nullptr",
+                "set_color_nullptr"
             ]
         }, {
             "id": "Pipeline",

diff --git a/test/addons/src/Doc.c b/test/addons/src/Doc.c
@@ -62,3 +62,73 @@ void Doc_get_set_link(void) {
 
     ecs_fini(world);
 }
+
+void Doc_set_name_nullptr(void) {
+    ecs_world_t *world = ecs_init();
+
+    ecs_entity_t e = ecs_new_id(world);
+
+    ecs_doc_set_name(world, e, "foo");
+    test_assert( ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsName));
+
+    ecs_doc_set_name(world, e, NULL);
+    test_assert( !ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsName));
+
+    ecs_fini(world);
+}
+
+void Doc_set_brief_nullptr(void) {
+    ecs_world_t *world = ecs_init();
+
+    ecs_entity_t e = ecs_new_id(world);
+
+    ecs_doc_set_brief(world, e, "foo");
+    test_assert( ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocBrief));
+
+    ecs_doc_set_brief(world, e, NULL);
+    test_assert( !ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocBrief));
+
+    ecs_fini(world);
+}
+
+void Doc_set_detail_nullptr(void) {
+    ecs_world_t *world = ecs_init();
+
+    ecs_entity_t e = ecs_new_id(world);
+
+    ecs_doc_set_detail(world, e, "foo");
+    test_assert( ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocDetail));
+
+    ecs_doc_set_detail(world, e, NULL);
+    test_assert( !ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocDetail));
+
+    ecs_fini(world);
+}
+
+void Doc_set_link_nullptr(void) {
+    ecs_world_t *world = ecs_init();
+
+    ecs_entity_t e = ecs_new_id(world);
+
+    ecs_doc_set_link(world, e, "foo");
+    test_assert( ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocLink));
+
+    ecs_doc_set_link(world, e, NULL);
+    test_assert( !ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocLink));
+
+    ecs_fini(world);
+}
+
+void Doc_set_color_nullptr(void) {
+    ecs_world_t *world = ecs_init();
+
+    ecs_entity_t e = ecs_new_id(world);
+
+    ecs_doc_set_color(world, e, "foo");
+    test_assert( ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocColor));
+
+    ecs_doc_set_color(world, e, NULL);
+    test_assert( !ecs_has_pair(world, e, ecs_id(EcsDocDescription), EcsDocColor));
+
+    ecs_fini(world);
+}
diff --git a/test/addons/src/main.c b/test/addons/src/main.c
@@ -475,6 +475,11 @@ void Doc_get_entity_name(void);
 void Doc_get_set_brief(void);
 void Doc_get_set_detail(void);
 void Doc_get_set_link(void);
+void Doc_set_name_nullptr(void);
+void Doc_set_brief_nullptr(void);
+void Doc_set_detail_nullptr(void);
+void Doc_set_link_nullptr(void);
+void Doc_set_color_nullptr(void);
 
 // Testsuite 'Pipeline'
 void Pipeline_system_order_same_phase(void);
@@ -3402,6 +3407,26 @@ bake_test_case Doc_testcases[] = {
     {
         "get_set_link",
         Doc_get_set_link
+    },
+    {
+        "set_name_nullptr",
+        Doc_set_name_nullptr
+    },
+    {
+        "set_brief_nullptr",
+        Doc_set_brief_nullptr
+    },
+    {
+        "set_detail_nullptr",
+        Doc_set_detail_nullptr
+    },
+    {
+        "set_link_nullptr",
+        Doc_set_link_nullptr
+    },
+    {
+        "set_color_nullptr",
+        Doc_set_color_nullptr
     }
 };
 
@@ -7520,7 +7545,7 @@ static bake_test_suite suites[] = {
         "Doc",
         NULL,
         NULL,
-        5,
+        10,
         Doc_testcases
     },
     {

diff --git a/test/api/project.json b/test/api/project.json
@@ -1715,7 +1715,9 @@
                 "page_iter_w_fini",
                 "worker_iter_w_fini",
                 "rule_page_iter_w_fini",
-                "rule_worker_iter_w_fini"
+                "rule_worker_iter_w_fini",
+                "to_str_before_next",
+                "to_str"
             ]
         }, {
             "id": "Pairs",

diff --git a/test/api/src/Iter.c b/test/api/src/Iter.c
@@ -2182,3 +2182,47 @@ void Iter_rule_worker_iter_w_fini(void) {
 
     ecs_fini(world);
 }
+
+void Iter_to_str_before_next(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_TAG(world, Tag);
+
+    ecs_query_t *q = ecs_query_new(world, "Tag");
+    test_assert(q != NULL);
+
+    ecs_iter_t it = ecs_query_iter(world, q);
+    char *str = ecs_iter_str(&it);
+    test_assert(str == NULL);
+    test_bool(false, ecs_query_next(&it));
+
+    ecs_fini(world);
+}
+
+void Iter_to_str(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_TAG(world, Tag);
+
+    ecs_entity_t e = ecs_new_entity(world, "foo");
+    ecs_add(world, e, Tag);
+
+    ecs_query_t *q = ecs_query_new(world, "Tag");
+    test_assert(q != NULL);
+
+    ecs_iter_t it = ecs_query_iter(world, q);
+    test_bool(true, ecs_query_next(&it));
+    char *str = ecs_iter_str(&it);
+    test_assert(str != NULL);
+    test_str(str, 
+        "id:  Tag\n"
+        "src: 0\n"
+        "set: true\n"
+        "this:\n"
+        "    - foo\n"
+    );
+    ecs_os_free(str);
+    test_bool(false, ecs_query_next(&it));
+
+    ecs_fini(world);
+}