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

Prevent concatenation for string constants in headers? #2425

Open
kanashimia opened this issue Aug 27, 2024 · 2 comments
Open

Prevent concatenation for string constants in headers? #2425

kanashimia opened this issue Aug 27, 2024 · 2 comments
Assignees
Labels

Comments

@kanashimia
Copy link

So I've encountered the most stupid thing ever: https://infosec.exchange/@kanashimia/113033538870920565
Because string constants such as VK_KHR_VIDEO_ENCODE_QUEUE_EXTENSION_NAME are defined using preprocessor you can concatenate them accidentally.
Maybe it would be appropriate to wrap them in a parenthesis.

Quoting from the post:

GCC doesn't warn you about string concatenations at all.
Clang has -Wstring-concatenation, but that doesn't work through defines.
So IDE based on clangd won't warn about it too.

If these strings were defined in parenthesis as
# define FOO ("foo")
then such problems couldn't occur, you can't concatenate ("foo") ("bar").
Although not sure if that causes any other problems, maybe some people do rely on the ability to concatenate those strings?
Who knows.

Spec is written in a weird way where it contains quotes value=""VK_KHR_video_encode_queue"", I guess just for an easier C generation, but ideally this should be fixed just for C headers, it isn't appropriate to change the spec because of this and break other binding generators.

This is technically a breaking change at the headers API level, but do people really rely on this?
What other possible problems can wrapping string in () cause?

I just throw this out for thinking and laughs, overall the issue is minor and may be ignored, you decide.

@oddhack
Copy link
Contributor

oddhack commented Aug 28, 2024

I'll take a look at tweaking the header generator script to do this and link it to this issue when ready - it may be a while. We'd like to get a little soak time with a sample header before actually pulling the switch on shipping a change like this, since while it seems benign, it's impossible to know what odd downstream uses people might be making of the headers.

@oddhack oddhack self-assigned this Aug 28, 2024
@oddhack oddhack added the Header label Aug 28, 2024
@oddhack
Copy link
Contributor

oddhack commented Aug 28, 2024

Re the ", in retrospect we should have tagged the strings with a type attribute so the generator could add them. As with some other suboptimal choices in the XML, this is essentially impossible to change now because there are so many downstream consumers doing their own XML parsing and processing.

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

No branches or pull requests

2 participants