Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove reinterpret_casts when assigning ecs_ctx_free_t which were breaking aliasing rules #1416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gaspard--
Copy link
Contributor

The code was casting functions pointers of the signature void (*)(T *) to void (*)(void *) and then calling them on a void *. This breaks aliasing rules and is undefined behaviour. This commit removes these casts by changing the template to take a void * directly.

This was caught by -fsanitize=undefined under clang.
I'm suspicious of a couple other places which seem to reinterpret_cast function pointers, but since I have no concrete evidence I'd rather not touch them.

…eaking aliasing rules

The code was casting functions pointers of the signature `void (*)(T *)` to `void (*)(void *)`
and then calling them on a `void *`. This breaks aliasing rules and is undefined behaviour.
This commit removes these casts by changing the template to take a `void *` directly.
@Gaspard-- Gaspard-- force-pushed the fix_reinterpret_cast_aliasing branch from 0381b92 to a7bb259 Compare October 31, 2024 10:25
@SanderMertens
Copy link
Owner

The fix seems more complicated than #1416

This comment in #1417 suggests that this is not a correct fix. Should this PR be closed?

@Gaspard--
Copy link
Contributor Author

Sorry if I was unclear. In this case, I changed the function signature of the function we assign to the callback itself to void (*)(void *), so that we don't need a reinterpret_cast at all. This is possible because we provide the callback in flecs itself.

In the case of the linked issue, the user provides the callback, so it's not possible to change the signature trivially without harming user experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants