(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.
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.From 3179454b77cfe1b818046692e4068e594b6cd8cb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Mon, 5 Oct 2026 07:52:57 +0200
Subject: [PATCH v1.5] Add compiler option -fstrict-flex-arrays=1
Currently, every trailing array regardless of its declared bound is
exempt from -Warray-bounds, __builtin_object_size/_FORTIFY_SOURCE, and
-fsanitize=bounds. -fstrict-flex-arrays=1 says that only arrays
declared with bounds [], [0], or [1] can be flexible array
members (which are then mostly exempt from array bounds checking); for
all other arrays, the declared size is taken as is for bounds
checking, which is what we want.
This applies to a large number of structs in the tree, for example
- struct pg_hmac_ctx
- fe_scram_state
- struct XidCache
- struct RelPathStr
...
which can now be checked properly.
Ideally, we'd want to use the maximum level -fstrict-flex-arrays=3,
which would also disqualify flexible array members declared with
bounds [0] or [1]. But we do need the [1] variant 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
Also:
- struct sqlda_struct (bound by external API specification)
Turning on -fstrict-flex-arrays=3 for these would conjure undefined
behavior.
One piece of code needs to be adjusted: JsonValueList has an array
declared as [2] (via BASE_JVL_ITEMS) but addresses it as a
flexible-length array in some circumstances. 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. We can't make it a fully flexible-array
member [], because JsonValueList is also used for a field in the
middle of JsonTablePlanState, which is not allowed in standard C.
There is one structure with a genuine trailing one-element array:
- WordEntryPosVector1
By staying with -fstrict-flex-arrays=1 we miss out on bounds checking
on that one.
---
configure | 92 +++++++++++++++++++++++++++
configure.ac | 3 +
meson.build | 2 +
src/backend/utils/adt/jsonpath_exec.c | 16 ++++-
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 6b05d4c84ab..8d29a0bf6a0 100755
--- a/configure
+++ b/configure
@@ -6421,6 +6421,98 @@ if test
x"$pgac_cv_prog_CXX_cxxflags__fexcess_precision_standard" = x"yes"; then
fi
+ # Treat only arrays with [], [0], or [1] as flexible array members.
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports
-fstrict-flex-arrays=1, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -fstrict-flex-arrays=1, for
CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -fstrict-flex-arrays=1"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1=yes
+else
+ pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__fstrict_flex_arrays_1" = x"yes"; then
+ CFLAGS="${CFLAGS} -fstrict-flex-arrays=1"
+fi
+
+
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports
-fstrict-flex-arrays=1, for CXXFLAGS" >&5
+$as_echo_n "checking whether ${CXX} supports -fstrict-flex-arrays=1, for
CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -fstrict-flex-arrays=1"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS
conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+ pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1=yes
+else
+ pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext
$LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__fstrict_flex_arrays_1" = x"yes"; then
+ CXXFLAGS="${CXXFLAGS} -fstrict-flex-arrays=1"
+fi
+
+
# Optimization flags for specific files that benefit from loop unrolling
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports
-funroll-loops, for CFLAGS_UNROLL_LOOPS" >&5
$as_echo_n "checking whether ${CC} supports -funroll-loops, for
CFLAGS_UNROLL_LOOPS... " >&6; }
diff --git a/configure.ac b/configure.ac
index 6864268eaa0..36e4f9ca0cf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -610,6 +610,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
# Disable FP optimizations that cause various errors on gcc 4.5+ or maybe
4.6+
PGAC_PROG_CC_CFLAGS_OPT([-fexcess-precision=standard])
PGAC_PROG_CXX_CFLAGS_OPT([-fexcess-precision=standard])
+ # Treat only arrays with [], [0], or [1] as flexible array members.
+ PGAC_PROG_CC_CFLAGS_OPT([-fstrict-flex-arrays=1])
+ PGAC_PROG_CXX_CFLAGS_OPT([-fstrict-flex-arrays=1])
# Optimization flags for specific files that benefit from loop unrolling
PGAC_PROG_CC_VAR_OPT(CFLAGS_UNROLL_LOOPS, [-funroll-loops])
# Optimization flags for specific files that benefit from vectorization
diff --git a/meson.build b/meson.build
index d05634fef0d..41aa306f81a 100644
--- a/meson.build
+++ b/meson.build
@@ -2185,6 +2185,8 @@ common_functional_flags = [
# Disable optimizations that assume no overflow; needed for gcc 4.3+
'-fwrapv',
'-fexcess-precision=standard',
+ # Treat only arrays with [], [0], or [1] as flexible array members.
+ '-fstrict-flex-arrays=1',
# Without -fpch-deps gcc emits dependencies that are insufficient for ccache
# to trigger a rebuild when the precompiled header changes. We could make
# this depend on using gcc and precompiled headers being enabled, but that's
diff --git a/src/backend/utils/adt/jsonpath_exec.c
b/src/backend/utils/adt/jsonpath_exec.c
index 2191d67bfc0..a3d0a541f28 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -174,7 +174,21 @@ typedef struct JsonValueList
int maxitems; /* allocated length of
items[] */
struct JsonValueList *next; /* => next chunk, if any */
struct JsonValueList *last; /* => last chunk (only valid in base chunk)
*/
- JsonbValue items[BASE_JVL_ITEMS];
+ union
+ {
+ /*
+ * To satisfy -fstrict-flex-arrays=1, we need one union member
that
+ * makes space for the base chunk and one member that is
effectively
+ * declared as a flexible-array member, otherwise you'd get
+ * -fsanitize=undefined errors. Note that we can't declare
items as
+ * [FLEXIBLE_ARRAY_MEMBER], because JsonValueList is used in a
+ * non-last field of JsonTablePlanState, and that's not allowed
with
+ * flexible-array members. But using [1] works as long as we
don't go
+ * higher than -fstrict-flex-arrays=1.
+ */
+ JsonbValue base_items[BASE_JVL_ITEMS];
+ JsonbValue items[1];
+ };
} JsonValueList;
/* State data for iterating through a JsonValueList */
base-commit: 675dd940bc459f3e1748ffcd82c48f24c29b2a80
--
2.56.0
