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

Destructors msg_fddata_release and str_release being called via void (*)(void *) is an undefined behaviour. #6

Open
shamefulCake1 opened this issue Sep 5, 2024 · 3 comments

Comments

@shamefulCake1
Copy link

shamefulCake1 commented Sep 5, 2024

As far as I understand, calling a function via incompatible function pointers (even if the argument types are compatible) is undefined behaviour.

A related (but for c++) question discussed here: https://stackoverflow.com/questions/43220926/call-to-function-unknown-through-pointer-to-incorrect-function-type

Would it be possible to fix this somehow?

libslack/msg.c:357:4: runtime error: call to function msg_fddata_release through pointer to incorrect function type 'void (*)(void *)'
~/repos/daemon/libslack/msg.c:514: note: msg_fddata_release defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libslack/msg.c:357:4 
libslack/list.c:340:4: runtime error: call to function str_release through pointer to incorrect function type 'void (*)(void *)'
~/repos/daemon/libslack/str.c:874: note: str_release defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libslack/list.c:340:4 
@raforg
Copy link
Owner

raforg commented Sep 10, 2024

Hi, Sorry but I don't think it's fixable. That's how to do generics in C (i.e. rely on the the ability of void * to act as a placeholder for any other pointer). I know of no other way to do it. But the good news is that, even though it's undefined behaviour according to UBSAN, it has never not worked on any operating system I have access to in 25 years. In fact, the only way I can think of for it to not work is if some function pointers were 64-bit while other function pointers were 128-bit. Hopefully, that will never happen. But even if it does happen, whoever tries to use the differently-sized function pointer should find out pretty quickly that it doesn't work and find their own solution (like maybe a wrapper function with the right-sized function pointer).

@shamefulCake1
Copy link
Author

It's there no way to use some clever type-casting, or doing a union of different types?

@raforg
Copy link
Owner

raforg commented Sep 10, 2024

Not a union (but keep reading for the good news). A union would require defining a union in the library that contains every function pointer type that will ever be used by any client of the library. That's not possible to do without requiring every user of the library to modify the union in the library whenever they want to use a new function with the library. And there is already clever type casting but it seems that casting function pointer types is undefined (who knew?).

Actually, I suspect it could be fixed by changing the signatures of msg_fddata_release and str_release to take a void * parameter, but I'm not willing to do that. That would take away type checking from those functions when they are used explicitly and not through a generic function pointer.

Actually, it could probably also be fixed by defining an additional wrapper function for each release function that is used generically. The wrapper function could have the correct signature to match the generic function pointer type, and the body of the wrapper function could call the underlying function and cast its own void * argument to the appropriate underlying type. That way, an undefined function pointer cast is replaced with a defined pointer cast.

Until it's actually broken, I'd rather not introduce extra function calls. Although it might be safe to assume that they would be inlined.

But that does sound like a good solution. The underlying release functions in the library stay as they are, and when some client code wants to use them with code expecting a generic function pointer, that client code can add a wrapper function for that purpose.

Actually the wrapper functions should be added to the library (at least for all the release functions which are most likely to need to be used in a generic context). And the wrapper functions probably need to be in the library itself if there is to be any chance of them being inlined.

Thanks for not giving up! I will fix it. I recently added the ability to use ubsan analysis and I'm in the middle of a large increase in library test coverage, so presumably I'll see this in normal testing and will resolve it. But that work was interrupted quite some time ago and other things got in the way. But thanks for making me think more about this (although I don't think it's important, but maybe it will be when we have 128-bit computers). But I admit that it does feel good when there is no undefined behaviour. When I added ubsan analysis to another program of mine, it was great that the only undefined behaviour was a test that deliberately instructed the program to do arithmetic overflow.

@shamefulCake1 shamefulCake1 changed the title Destructors msg_fddata_release and str_release beind called via void (*)(void *) is an undefined behaviour. Destructors msg_fddata_release and str_release being called via void (*)(void *) is an undefined behaviour. Sep 10, 2024
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

No branches or pull requests

2 participants