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

build: enable -Wvla #4038

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

asadsa92
Copy link
Contributor

Enable -Wvla by default, if supported by the compiler, to warn about Variable Length Arrays (VLA). Misuse of VLAs can result in a stack overflow.

VLA can sometimes simplify things, so allow its use only if explicitly marked through a new v_vla_(type, name, count) macro.

Enable -Wvla by default, if supported by the compiler, to warn about
Variable Length Arrays (VLA). Misuse of VLAs can result in a stack overflow.

VLA can sometimes simplify things, so allow its use only if explicitly
marked through a new v_vla_(type, name, count) macro.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
@nigoroll
Copy link
Member

I am 👎🏽 because the suggested macro significantly decreases readability and I do not see any value in introducing a boilerplate which will just be applied wholesale. We need to think about VLAs and know what we are doing, and I do not see how this proposal makes this any easier or safer.

@nigoroll
Copy link
Member

Not sure if this is a good idea, but we could maybe have something like

#define ASSERT_VLA(x) assert(sizeof x < 1<<14))

reasoning: outside pcre, we should "always" have 32k stack free, so we could maybe trigger early to safeguard against stack overflow attacks.

@dridi
Copy link
Member

dridi commented Jan 23, 2024

I am 👎🏽 because the suggested macro significantly decreases readability and I do not see any value in introducing a boilerplate which will just be applied wholesale.

To be fair, this will trigger on future variable-length arrays and force us to think twice when we introduce some.

Also, this very pull request would actually be the occasion for us to review the legitimacy of existing instances, if anything.

@dridi dridi marked this pull request as draft January 23, 2024 09:11
@asadsa92
Copy link
Contributor Author

Just for completeness, the macro ended up looking like this piece of code: https://github.com/jemalloc/jemalloc/blob/dev/include/jemalloc/internal/jemalloc_internal_types.h#L126-L142

I did not bother to add support for pre C99, maybe this should be done as well?
I understand the point about "significantly decreases readability", but that was partly intended to discourage use of them :)

@nigoroll
Copy link
Member

but that was partly intended to discourage use of them :)

<sarcasm>yeah, bring back alloca()!</sarcasm>

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.

None yet

3 participants