varnish-cache icon indicating copy to clipboard operation
varnish-cache copied to clipboard

build: enable -Wvla

Open asadsa92 opened this issue 2 years ago • 5 comments

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.

asadsa92 avatar Jan 15 '24 13:01 asadsa92

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 avatar Jan 22 '24 18:01 nigoroll

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.

nigoroll avatar Jan 22 '24 18:01 nigoroll

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 avatar Jan 23 '24 09:01 dridi

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 :)

asadsa92 avatar Jan 23 '24 09:01 asadsa92

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

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

nigoroll avatar Jan 31 '24 12:01 nigoroll