#1453 Fix leak when deferring a batched set for a non trivial large c…

…omponent
SanderMertens · Dec 17, 2024 · 2ae21d6 · 2ae21d6
1 parent 0c70671
commit 2ae21d6
Show file tree

Hide file tree

Showing 5 changed files with 287 additions and 5 deletions.
diff --git a/distr/flecs.c b/distr/flecs.c
@@ -10647,15 +10647,17 @@ void flecs_cmd_batch_for_entity(
                         ecs_xtor_t dtor = ti->hooks.dtor;
                         if (dtor) {
                             dtor(cmd->is._1.value, 1, ti);
-                            cmd->is._1.value = NULL;
                         }
                     } else {
                         ecs_os_memcpy(ptr.ptr, cmd->is._1.value, ti->size);
                     }
 
+                    flecs_stack_free(cmd->is._1.value, cmd->is._1.size);
+                    cmd->is._1.value = NULL;
+
                     if (cmd->kind == EcsCmdSet) {
                         /* A set operation is add + copy + modified. We just did
-                         * the add the copy, so the only thing that's left is a 
+                         * the add and copy, so the only thing that's left is a 
                          * modified command, which will call the OnSet 
                          * observers. */
                         cmd->kind = EcsCmdModified;

diff --git a/src/entity.c b/src/entity.c
@@ -5023,15 +5023,17 @@ void flecs_cmd_batch_for_entity(
                         ecs_xtor_t dtor = ti->hooks.dtor;
                         if (dtor) {
                             dtor(cmd->is._1.value, 1, ti);
-                            cmd->is._1.value = NULL;
                         }
                     } else {
                         ecs_os_memcpy(ptr.ptr, cmd->is._1.value, ti->size);
                     }
 
+                    flecs_stack_free(cmd->is._1.value, cmd->is._1.size);
+                    cmd->is._1.value = NULL;
+
                     if (cmd->kind == EcsCmdSet) {
                         /* A set operation is add + copy + modified. We just did
-                         * the add the copy, so the only thing that's left is a 
+                         * the add and copy, so the only thing that's left is a 
                          * modified command, which will call the OnSet 
                          * observers. */
                         cmd->kind = EcsCmdModified;

diff --git a/test/core/project.json b/test/core/project.json
@@ -2116,6 +2116,12 @@
                 "defer_remove",
                 "defer_remove_two",
                 "defer_set",
+                "defer_set_large",
+                "defer_set_large_non_trivial",
+                "defer_set_non_trivial",
+                "defer_batched_set_large",
+                "defer_batched_set_large_non_trivial",
+                "defer_batched_set_non_trivial",
                 "defer_delete",
                 "defer_twice",
                 "defer_twice_in_progress",

diff --git a/test/core/src/Commands.c b/test/core/src/Commands.c
@@ -268,6 +268,248 @@ void Commands_defer_set(void) {
     ecs_fini(world);
 }
 
+static
+void dummy_xtor(void *ptr, ecs_size_t count, const ecs_type_info_t *ti) { }
+
+static
+void dummy_move(void *ptr, void *src, ecs_size_t count, const ecs_type_info_t *ti) { 
+    ecs_os_memcpy(ptr, src, count * ti->size);
+}
+
+typedef struct Large {
+    int32_t largeArray[2048];
+} Large;
+
+void Commands_defer_set_large(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Large);
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    Large *l = ecs_ensure(world, e, Large);
+    test_assert(l != NULL);
+    for (int i = 0; i < 2048; i ++) {
+        l->largeArray[i] = i;
+    }
+    ecs_modified(world, e, Large);
+
+    test_assert(!ecs_has(world, e, Large));
+    ecs_defer_end(world);
+
+    test_assert(ecs_has(world, e, Large));
+
+    {
+        const Large *l = ecs_get(world, e, Large);
+        test_assert(l != NULL);
+        for (int i = 0; i < 2048; i ++) {
+            test_int(l->largeArray[i], i);
+        }
+    }
+
+    ecs_fini(world);
+}
+
+void Commands_defer_set_large_non_trivial(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Large);
+
+    ecs_set_hooks(world, Large, {
+        .ctor = dummy_xtor,
+        .dtor = dummy_xtor,
+        .move = dummy_move
+    });
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    Large *l = ecs_ensure(world, e, Large);
+    test_assert(l != NULL);
+    for (int i = 0; i < 2048; i ++) {
+        l->largeArray[i] = i;
+    }
+    ecs_modified(world, e, Large);
+
+    test_assert(!ecs_has(world, e, Large));
+    ecs_defer_end(world);
+    test_assert(ecs_has(world, e, Large));
+
+    {
+        const Large *l = ecs_get(world, e, Large);
+        test_assert(l != NULL);
+        for (int i = 0; i < 2048; i ++) {
+            test_int(l->largeArray[i], i);
+        }
+    }
+
+    ecs_fini(world);
+}
+
+void Commands_defer_set_non_trivial(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Velocity);
+
+    ecs_set_hooks(world, Velocity, {
+        .ctor = dummy_xtor,
+        .dtor = dummy_xtor,
+        .move = dummy_move
+    });
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    ecs_set(world, e, Velocity, {1, 2});
+
+    test_assert(!ecs_has(world, e, Velocity));
+    ecs_defer_end(world);
+
+    test_assert(ecs_has(world, e, Velocity));
+
+    {
+        const Velocity *v = ecs_get(world, e, Velocity);
+        test_assert(v != NULL);
+        test_int(v->x, 1);
+        test_int(v->y, 2);
+    }
+
+    ecs_fini(world);
+}
+
+void Commands_defer_batched_set_large(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Position);
+    ECS_COMPONENT(world, Large);
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    ecs_set(world, e, Position, {10, 20});
+    Large *l = ecs_ensure(world, e, Large);
+    test_assert(l != NULL);
+    for (int i = 0; i < 2048; i ++) {
+        l->largeArray[i] = i;
+    }
+    ecs_modified(world, e, Large);
+
+    test_assert(!ecs_has(world, e, Position));
+    test_assert(!ecs_has(world, e, Large));
+    ecs_defer_end(world);
+
+    test_assert(ecs_has(world, e, Position));
+    test_assert(ecs_has(world, e, Large));
+
+    {
+        const Position *p = ecs_get(world, e, Position);
+        test_assert(p != NULL);
+        test_int(p->x, 10);
+        test_int(p->y, 20);
+
+        const Large *l = ecs_get(world, e, Large);
+        test_assert(l != NULL);
+        for (int i = 0; i < 2048; i ++) {
+            test_int(l->largeArray[i], i);
+        }
+    }
+
+    ecs_fini(world);
+}
+
+void Commands_defer_batched_set_large_non_trivial(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Position);
+    ECS_COMPONENT(world, Large);
+
+    ecs_set_hooks(world, Large, {
+        .ctor = dummy_xtor,
+        .dtor = dummy_xtor,
+        .move = dummy_move
+    });
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    ecs_set(world, e, Position, {10, 20});
+    Large *l = ecs_ensure(world, e, Large);
+    test_assert(l != NULL);
+    for (int i = 0; i < 2048; i ++) {
+        l->largeArray[i] = i;
+    }
+    ecs_modified(world, e, Large);
+
+    test_assert(!ecs_has(world, e, Position));
+    test_assert(!ecs_has(world, e, Large));
+    ecs_defer_end(world);
+
+    test_assert(ecs_has(world, e, Position));
+    test_assert(ecs_has(world, e, Large));
+
+    {
+        const Position *p = ecs_get(world, e, Position);
+        test_assert(p != NULL);
+        test_int(p->x, 10);
+        test_int(p->y, 20);
+
+        const Large *l = ecs_get(world, e, Large);
+        test_assert(l != NULL);
+        for (int i = 0; i < 2048; i ++) {
+            test_int(l->largeArray[i], i);
+        }
+    }
+
+    ecs_fini(world);
+}
+
+void Commands_defer_batched_set_non_trivial(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ECS_COMPONENT(world, Position);
+    ECS_COMPONENT(world, Velocity);
+
+    ecs_set_hooks(world, Velocity, {
+        .ctor = dummy_xtor,
+        .dtor = dummy_xtor,
+        .move = dummy_move
+    });
+
+    ecs_entity_t e = ecs_new(world);
+
+    ecs_defer_begin(world);
+
+    ecs_set(world, e, Position, {10, 20});
+    ecs_set(world, e, Velocity, {1, 2});
+
+    test_assert(!ecs_has(world, e, Position));
+    test_assert(!ecs_has(world, e, Velocity));
+    ecs_defer_end(world);
+
+    test_assert(ecs_has(world, e, Position));
+    test_assert(ecs_has(world, e, Velocity));
+
+    {
+        const Position *p = ecs_get(world, e, Position);
+        test_assert(p != NULL);
+        test_int(p->x, 10);
+        test_int(p->y, 20);
+
+        const Velocity *v = ecs_get(world, e, Velocity);
+        test_assert(v != NULL);
+        test_int(v->x, 1);
+        test_int(v->y, 2);
+    }
+
+    ecs_fini(world);
+}
+
 void Commands_defer_delete(void) {
     ecs_world_t *world = ecs_mini();
 

diff --git a/test/core/src/main.c b/test/core/src/main.c
@@ -2035,6 +2035,12 @@ void Commands_defer_add_two(void);
 void Commands_defer_remove(void);
 void Commands_defer_remove_two(void);
 void Commands_defer_set(void);
+void Commands_defer_set_large(void);
+void Commands_defer_set_large_non_trivial(void);
+void Commands_defer_set_non_trivial(void);
+void Commands_defer_batched_set_large(void);
+void Commands_defer_batched_set_large_non_trivial(void);
+void Commands_defer_batched_set_non_trivial(void);
 void Commands_defer_delete(void);
 void Commands_defer_twice(void);
 void Commands_defer_twice_in_progress(void);
@@ -10208,6 +10214,30 @@ bake_test_case Commands_testcases[] = {
         "defer_set",
         Commands_defer_set
     },
+    {
+        "defer_set_large",
+        Commands_defer_set_large
+    },
+    {
+        "defer_set_large_non_trivial",
+        Commands_defer_set_large_non_trivial
+    },
+    {
+        "defer_set_non_trivial",
+        Commands_defer_set_non_trivial
+    },
+    {
+        "defer_batched_set_large",
+        Commands_defer_batched_set_large
+    },
+    {
+        "defer_batched_set_large_non_trivial",
+        Commands_defer_batched_set_large_non_trivial
+    },
+    {
+        "defer_batched_set_non_trivial",
+        Commands_defer_batched_set_non_trivial
+    },
     {
         "defer_delete",
         Commands_defer_delete
@@ -11635,7 +11665,7 @@ static bake_test_suite suites[] = {
         "Commands",
         NULL,
         NULL,
-        143,
+        149,
         Commands_testcases
     },
     {