pi-hole / FTL

The Pi-hole FTL engine
https://pi-hole.net
Other
1.34k stars 187 forks source link

Compilation failure with GCC 14 #1915

Closed mbooth101 closed 2 months ago

mbooth101 commented 3 months ago

Versions

Latest HEAD of development-v6 branch, c835795a7bfdadee37a431d5e78337197b36383b

Platform

OS:

<mock-chroot> sh-5.2# lsb_release -i -r
Distributor ID: Fedora
Release:        40

Platform:

<mock-chroot> sh-5.2# uname -m
x86_64

Compiler

<mock-chroot> sh-5.2# gcc --version
gcc (GCC) 14.0.1 20240217 (Red Hat 14.0.1-0)

CMake:

<mock-chroot> sh-5.2# cmake --version
cmake version 3.28.2

Expected behavior

Successful build.

Actual behavior / bug

Build failure:

In file included from /builddir/build/FTL/src/capabilities.c:15:
/builddir/build/FTL/src/capabilities.c: In function ‘check_capability’:
/builddir/build/FTL/src/capabilities.c:28:46: error: ‘FTLcalloc’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
   28 |         cap_user_header_t hdr = calloc(sizeof(*hdr), capsize);
      |                                              ^

Steps to reproduce

Steps to reproduce the behavior:

Must have GCC 14 to reproduce:

$ export CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer '
$ export LDFLAGS='-Wl,-z,relro -Wl,--as-needed  -Wl,-z,pack-relative-relocs -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld-errors -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -Wl,--build-id=sha1 '
$ cmake -S . -B redhat-linux-build -DCMAKE_C_FLAGS_RELEASE:STRING=-DNDEBUG -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DBUILD_SHARED_LIBS:BOOL=ON
$ cmake --build redhat-linux-build --verbose

Additional context

In GCC 14 -Wextra now implies -Wcalloc-transposed-args

This warning is...

... about calls to allocation functions decorated with attribute "alloc_size" with two arguments, which use "sizeof" operator as the earlier size argument and don't use it as the later size argument. This is a coding style warning....

However, IMO there is nothing wrong with your usage of calloc() so I choose to suppress this warning by passing -Wno-calloc-transposed-args when GCC 14 is detected.

mbooth101 commented 3 months ago

I am locally appling a patch like this:

From a6b6d2d7cfd9774220ccd00d031e2f699afd3d29 Mon Sep 17 00:00:00 2001
From: Mat Booth <mat.booth@gmail.com>
Date: Sun, 17 Mar 2024 15:32:40 +0000
Subject: [PATCH] Fix compilation errors caused by GCC 14

---
 .../src/CMakeLists.txt                              | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/FTL-abcbcc58f5a63c21a11a4af2f0d5a664aa11ecfa/src/CMakeLists.txt b/FTL-abcbcc58f5a63c21a11a4af2f0d5a664aa11ecfa/src/CMakeLists.txt
index 185ba5d..f689760 100644
--- a/FTL-abcbcc58f5a63c21a11a4af2f0d5a664aa11ecfa/src/CMakeLists.txt
+++ b/FTL-abcbcc58f5a63c21a11a4af2f0d5a664aa11ecfa/src/CMakeLists.txt
@@ -155,11 +155,22 @@ else()
 set(EXTRAWARN_GCC13 "")
 endif()

+# Extra warnings flags available only in GCC 14 and higher
+# In GCC 14, -Wextra implies -Wcalloc-transposed-args however, there is
+# nothing wrong with the calls to calloc that it detects, so disable
+# that warning here
+if(CMAKE_C_COMPILER_VERSION VERSION_EQUAL 14 OR CMAKE_C_COMPILER_VERSION VERSION_GREATER 14)
+set(EXTRAWARN_GCC14 "-Wno-calloc-transposed-args")
+else()
+set(EXTRAWARN_GCC14 "")
+endif()
+
 set(EXTRAWARN "${EXTRAWARN_GCC6} \
                ${EXTRAWARN_GCC7} \
                ${EXTRAWARN_GCC8} \
                ${EXTRAWARN_GCC12} \
-               ${EXTRAWARN_GCC13}")
+               ${EXTRAWARN_GCC13} \
+               ${EXTRAWARN_GCC14}")

 # Remove extra spaces from EXTRAWARN
 string(REGEX REPLACE " +" " " EXTRAWARN "${EXTRAWARN}")
-- 
2.41.0
DL6ER commented 3 months ago

Thanks for your comments, I haven't tried building on such a recent GCC version myself. I'm actually torn about the solution you suggest - yes, sure, there isn't anything wrong with our code. The warning wants to ensure we are using calloc() as it was designed and that means the first argument is nmem and the second size. Even when it makes no functional difference, one could also argument that the "real" fix would be

-cap_user_header_t hdr = calloc(sizeof(*hdr), capsize);
+cap_user_header_t hdr = calloc(capsize, sizeof(*hdr));

What do you think? I'm always trying to try to comply with compiler warnings whenever possible (and meaningful). It seems meaningful here because (warning: rusty partially unreliable memory excerpt ahead!) I recall that calloc may be implemented in a way that ensures alignment matching the size parameter. So the order of the arguments can actually make a difference and the mere fact that someone did the work adding warning rules to GCC 14 (combined with that this has been accepted) tells me that there are good reasons to use the function as defined :-)

DL6ER commented 3 months ago

For reference: https://github.com/gcc-mirror/gcc/commit/e7dd72aefed851d11655aa301d6e394ec9805e0d

mbooth101 commented 3 months ago

I am happy to see it fixed in any manner you think is best -- I am not a GCC expert, nor am I a FTL expert -- but your fix may indeed be better if only because not disabling the warning would prevent more infractions from sneaking into the code base. :-)

DL6ER commented 3 months ago

You could test swapping the arguments locally to see if anything else comes up. As GCC 14 has not even been officially released, I'm a bit hesitant to invest time into creating/installing a full testing environment yet. We may simply wait for alpine:edge to eventually pick it up once finalized.

github-actions[bot] commented 2 months ago

This issue is stale because it has been open 30 days with no activity. Please comment or update this issue or it will be closed in 5 days.

pralor-bot commented 1 month ago

This issue has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/certificate-domain-mismatch-warning-behind-reverse-proxy/70521/3