| From: | "Tristan Partin" <tristan(at)partin(dot)io> |
|---|---|
| To: | "Peter Eisentraut" <peter(at)eisentraut(dot)org> |
| Cc: | "Greg Burd" <greg(at)burd(dot)me>, "pgsql-hackers" <pgsql-hackers(at)postgresql(dot)org>, "David Geier" <geidav(dot)pg(at)gmail(dot)com> |
| Subject: | Re: Add compiler option -fstrict-flex-arrays=1 |
| Date: | 2026-10-27 15:51:39 |
| Message-ID: | DMFQOFXPQEOY.2YAWETGQMEWH5@partin.io |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon Oct 19, 2026 at 10:27 AM UTC, Peter Eisentraut wrote:
> (I already posted this patch in another thread [0], but since this was
> under another topic, I'm posting it again here with a clearer subject.)
>
> [0]:
> https://www.postgresql.org/message-id/flat/DLRI3Q0C0MR2.1V3TOIS3N16FJ%40partin.io
>
> Compilers can check object sizes in various ways. For example, there is
> -Warray-bounds, __builtin_object_size, which drives _FORTIFY_SOURCE, and
> also -fsanitize=bounds, which is part of ubsan.
>
> One perhaps surprising aspect of that is that both gcc and clang by
> default assume that any trailing array in a struct could be used as a
> flexible array member, independent of its declared size, and therefore
> don't believe the size declaration and thus exempt that array from all
> those checks.
>
> This patch fixes that. Below is a relatively elaborate explanation, but
> the patch is actually small.
>
> Here is an example of such a struct:
>
> typedef struct
> {
> ...
>
> char *server_final_message;
> uint8 ServerSignature[SCRAM_MAX_KEY_LEN];
> } fe_scram_state;
>
> (SCRAM_MAX_KEY_LEN is 32.)
>
> Because of the default assumption mentioned above, the compilers don't
> check that code doesn't overrun the ServerSignature field.
>
> There are many other structs in our code that are similarly affected.
>
> I suppose this behavior is to accommodate legacy code, but our code uses
> C99 flexible array members correctly (mostly, see below), so we can make
> these checks more strict and get more benefit from the various bounds
> checking mechanisms.
>
> gcc and clang have the option -fstrict-flex-arrays [1] for this.
>
> [1]:
> https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html#index-fstrict-flex-arrays
>
> Ideally, we'd want to use the maximum level -fstrict-flex-arrays=3,
> which would only allow flexible array members declared with the C
> standard "[]". But we do need to permit flexible array members declared
> with "[1]" in a few cases because standard C does not allow flexible
> array members as the only element in a struct. Affected are:
>
> - struct RevmapContents
> - struct ReplicationSlotCtlData
> - union pgresult_data
>
> (Some of these have comments "It should be declared
> [FLEXIBLE_ARRAY_MEMBER], but for some reason you can't do that in an
> otherwise-empty struct.")
>
> There is also:
>
> - struct sqlda_struct
>
> which, as I understand it, is bound by external API specification, so
> not easy to change.
>
> If we turned on -fstrict-flex-arrays=3, these would become undefined
> behavior and trigger the ubsan sanitizer.
>
> If we use the lower level -fstrict-flex-arrays=1, then flexible-array
> members declared as "[]", "[0]", and "[1]" are permitted, but all the
> obviously-not-flexible-array-members like the fe_scram_state example
> above can now be bounds checked properly. The only struct in the code
> that genuinely has a trailing one-element array is WordEntryPosVector1,
> and so we would miss bounds checking on that one, but I think we can
> live with that.
We could define a pg_attribute_strict_flex_array() macro and add it to
the entry in WordEntryPosVector1. We can add a comment to the macro
indicating how and why it should be used since it'll likely be a little
non-obvious, but we could continue to keep bounds checking on
WordEntryPosVector1.
> There is one piece of code that still needs fixing to permit
> -fstrict-flex-arrays=1, which was the topic of the [0] thread:
> JsonValueList has an array declared as "[2]" (via BASE_JVL_ITEMS) but
> addresses it as a flexible-length array in some circumstances. My
> proposal included here is to change this to an anonymous union with one
> branch having the original size "[2]", to keep local variables sized the
> same as before, and one branch of size "[1]", which effectively makes
> this a flexible-array member under -fstrict-flex-arrays=1.
Assuming you think my previous suggestion is reasonable, I think the
patch with my proposed change is good to go. Otherwise, the patch as is
will is also good.
--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ayoub Kazar | 2026-10-27 15:52:04 | Re: [PROPOSAL] Expand OR clauses in joins to UNION ALL paths |
| Previous Message | Rahila Syed | 2026-10-27 14:59:37 | Re: Fix concurrent-abort detection for subtransactions of prepared transactions |