varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.56k stars 366 forks source link

build: enable -Wvla #4038

Open asadsa92 opened 5 months ago

asadsa92 commented 5 months ago

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 vvla(type, name, count) macro.

nigoroll commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

asadsa92 commented 5 months ago

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 commented 5 months ago

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

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