SanderMertens / flecs

A fast entity component system (ECS) for C & C++
https://www.flecs.dev
Other
6.47k stars 454 forks source link

ecs_entity_init with empty separator fails when entity with that name already exists #1038

Closed copygirl closed 1 year ago

copygirl commented 1 year ago

Describe the bug ecs_entity_init can be called with an empty desc.sep to prevent tokenization of the given desc.name into a path. However, when the function is called while an entity with the given name already exists, ecs_entity_init calls ecs_get_path_w_sep to ensure that the given path matches the entity's path relative to the current scope. Of course, ecs_get_path_w_sep does not work when passing an empty separator, causing a sep[0] != 0 INVALID_PARAMETER assert.

To Reproduce I actually ran into this while porting Flecs' tests to my Zig wrapper. So here is an adjusted version of Entity_find_id_name_match that will cause this assert. (To run this standalone one would have to remove the test functions.)

ecs_world_t *world = ecs_mini();

ecs_entity_t e = ecs_entity_init(world, &(ecs_entity_desc_t){
    .name = "foo",
});
test_assert(e != 0);
test_str(ecs_get_name(world, e), "foo");

ecs_entity_t r = ecs_entity_init(world, &(ecs_entity_desc_t){
    .id = e,
    .name = "foo",
    .sep = "", // <-- This line was added.
});
test_assert(r != 0);
test_assert(r == e);

ecs_fini(world);

Suggested Fix Presumably the fix would look something along the lines of ..

if (!sep[0]) {
    /* Separator is empty, so no tokenization should occur */
    path = ecs_get_name(world, result);
} else {
    ecs_size_t root_sep_len = root_sep ? ecs_os_strlen(root_sep) : 0;
    if (root_sep && !ecs_os_strncmp(name, root_sep, root_sep_len)) {
        /* Fully qualified name was provided, so make sure to
         * compare with fully qualified name */
        path = ecs_get_path_w_sep(world, 0, result, sep, root_sep);
    } else {
        /* Relative name was provided, so make sure to compare with
         * relative name */
        path = ecs_get_path_w_sep(world, scope, result, sep, "");
    }
}

However ecs_get_path_w_sep allocates, and needs to be freed, whereas ecs_get_name does not. With my limited knowledge I'm not entirely sure how to structure the code to allow for conditional freeing of path, due to one being char * and the other const char * and ecs_os_free not accepting const pointers. Otherwise I'd have loved to submit this as a PR instead.

The above code assumes that if sep is empty, root_sep should also be ignored, which makes sense to me.

edit: I suppose an easy way to go about it would be to just have two separate branches, each doing their own ecs_os_strcmp check. This is what I did in my dev environment temporarily to fix the bug.

SanderMertens commented 1 year ago

Fixed!