core: Use flexible array member if compiler has support.
Concern Raised3c1c98a43413
Actions

Description

core: Use flexible array member if compiler has support.

* configure.ac (AC_C_FLEXIBLE_ARRAY_MEMBER): Add.
* src/engine-gpg.c (struct arg_and_data_s): Use FLEXIBLE_ARRAY_MEMBER.
(_add_arg): Use offsetof instead of sizeof.
(add_data): Likewise.

Before this fix, GCC 11 warns (with its bound checking feature).

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>

Details

Auditors

• werner

Provenance

• gniibe

Authored on Aug 13 2021, 7:52 AM

Parents

rM7cfc93193d53: core: Fix results returned by gpgme_data_* functions

Branches

Unknown

Tags

Unknown

Event Timeline

• gniibe committed rM3c1c98a43413: core: Use flexible array member if compiler has support. (authored by • gniibe).Aug 13 2021, 7:52 AM

• werner raised a concern with this commit.Aug 13 2021, 11:46 AM

• werner added a subscriber: • werner.

• werner added inline comments.

/src/engine-gpg.c
62	I consider the FLEXIBLE_ARRAY_MEMBER thingy a Bad Thing and bad for security. Yes, the warning is annoying but a compiler should be able to detect the pattern and not emit a warning for this. The odds that struct { int foo; char bar[1]; } is not a "flexible array" but bar[1] being a one element char array are so minimal that such code will have otehr bugs as weel. OTOH, this pattern in such a wide spread use for nearly 50 years that you can't simply define it away. The problem with bar[] and bar[1] is that the former does not reserve a single byte but the latter does. So the common pattern malloc (sizeof (struct something) + strlen (string)) will lead to a buffer overflow if bar[] is used instead of bar[1]. I don't want to check all code, and worse the mental pattern on how to allocate such an object, in million of lines of code. Just for the samll benefit that is certain corner case you could save a single byte (not considering padding). I would leave with a compiler warning or - if really needed - add some attribute to the final struct member to declare that it is "flexible array".

This commit now has outstanding concerns.Aug 13 2021, 11:46 AM

There are two things here.
(1) Use of [] (FLEXIBLE_ARRAY_MEMBER)
(2) Use of offsetof (instead of sizeof) for computation of size of allocation.

My point was basically (2). Use of offsetof is less confusing (and less error), with no counting the size by 1. Using 1 for calculation with string explicitly is better.

Then, after the change of (2), I found that in some situation (like this gpgme case), compiler (GCC 11) cannot consider it as use of flexible array when it's [1].
It seems that it occurs when the structure can be aligned to memory with [] but not with [1]. It will be correctly detected, hopefully, perhaps.

I'll revert the change for (1) (with the hope of future compiler). I think that the original intention of (2) is relevant.

• gniibe mentioned this in T5556: Use of offsetof is better for allocation of flexible array.Aug 16 2021, 10:17 AM