search

pi-hole / FTL

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

FTL v5.3.1 fails to build with gcc 10.2.x #950

Closed ganto closed 3 years ago

ganto commented 3 years ago

Platform

I'm aware that Gentoo is not a supported platform but due to the nature of the bug it might be reproducible on other platforms supporting gcc-10.2.x too. I tried to reproduce it on Fedora 32 which unfortunately failed because of a missing libidn-static (libidn.a)

Expected behavior

FTL build completes successful.

Actual behavior / bug

$ gcc --version
gcc (Gentoo 10.2.0-r3 p4) 10.2.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Error due to potential string truncation:

$ ./build.sh
[...]
Building Pi-hole FTL daemon                                    
   - Branch: master                                            
   - Architecture: x86_64 (compiled locally)
   - Version: v5.3.1
   - Tag: v5.3.1
   - Hash: e1db31d
   - Commit date: 2020-11-28 21:59:12 +0100
[...]
[ 50%] Building C object src/dnsmasq/CMakeFiles/dnsmasq.dir/inotify.c.o                                                                                        
[ 50%] Building C object src/dnsmasq/CMakeFiles/dnsmasq.dir/ipset.c.o          
In file included from /usr/include/string.h:519,                              
                 from /usr/include/sys/un.h:37,                                                                                                                
                 from /home/ganto/FTL/src/dnsmasq/dnsmasq.h:105,           
                 from /home/ganto/FTL/src/dnsmasq_interface.c:12:
In function 'strncpy',
    inlined from '_FTL_new_query' at /home/ganto/FTL/src/dnsmasq_interface.c:558:3:
/usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy' output may be truncated copying 45 bytes from a string of length 45 [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]

Steps to reproduce

If you want to reproduce it on Gentoo you can use the feature/pi-hole branch from my linuxmonk-overlay.

Otherwise I would expect any other Linux distribution that meets the requirements and a recent enough gcc version to fail with the same error.

Additional context

This is not a bug that affects the end users but sooner or later you might step over it too. Note that the same code compiles fine when using gcc-9.3.0 on Gentoo. Also FTL v5.2 compiled fine with gcc-10.2.0.

schorschfunke commented 3 years ago

It's fixed in the next version 5.3.2

ganto commented 3 years ago

I just tested with the feature/v5.3.2 branch but it seems this issue is not (yet) resolved:

$ git checkout release/v5.3.2                                                                                                               
Branch 'release/v5.3.2' set up to track remote branch 'release/v5.3.2' from 'origin'. 
Switched to a new branch 'release/v5.3.2'                                  
$ ./build.sh                                                                                                                                
-- The C compiler identification is GNU 10.2.0                               
-- Detecting C compiler ABI info                                             
-- Detecting C compiler ABI info - done                                    
-- Check for working C compiler: /usr/bin/cc - skipped                  
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Check if compiler accepts -pthread
-- Check if compiler accepts -pthread - yes
-- Found Threads: TRUE  
-- Building FTL with readline support: NO
-- Embedded LUA will use readline for history: NO
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ganto/FTL/cmake
[...]
Building Pi-hole FTL daemon
   - Branch: release/v5.3.2
   - Architecture: x86_64 (compiled locally)
   - Version: v5.3.1-8-gd25f05d
   - Tag: v5.3.1
   - Hash: d25f05d
   - Commit date: 2020-11-29 13:21:24 +0100
[...]
 47%] Building C object src/dnsmasq/CMakeFiles/dnsmasq.dir/domain.c.o
[ 47%] Built target tre-regex
[ 47%] Building C object src/dnsmasq/CMakeFiles/dnsmasq.dir/dump.c.o
[ 47%] Building C object src/CMakeFiles/FTL.dir/gc.c.o
In file included from /usr/include/string.h:519,
                 from /usr/include/sys/un.h:37,
                 from /home/ganto/FTL/src/dnsmasq/dnsmasq.h:105,
                 from /home/ganto/FTL/src/dnsmasq_interface.c:12:
In function 'strncpy',
    inlined from '_FTL_new_query' at /home/ganto/FTL/src/dnsmasq_interface.c:558:3:
/usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy' output may be truncated copying 45 bytes from a string of length 45 [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
DL6ER commented 3 years ago

You are right, this is not fixed, however, it also isn't strictly an error.

man strncpy says:

char *strcpy(char *dest, const char *src); char *strncpy(char *dest, const char *src, size_t n);

[....]

The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

A missing null-termination is expected and the next line is taking care of this https://github.com/pi-hole/FTL/blob/e1db31de09a5177459690118f55513f9d8fad2bd/src/dnsmasq_interface.c#L558-L559 so the gcc warning is somewhat incorrect here. We can, of course, change this everywhere, however, as my local gcc isn't complaining, it'll take some time to find all the occurrences by hand (which is also why I haven't done this so far). I may try with your docker suggestion. I guess it's the only really meaningful way to address this.

ganto commented 3 years ago

This seems to be an known issue popping up here and there since GCC 8: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88780

I don't fully understand from the bug report if the assignment of the \0 termination in the next line should actually be detected. Strange that it doesn't consistently report an error.

As you're not doing anything wrong here this ugly work-around could be added:

diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c
index 340aed1..4fed8d8 100644
--- a/src/dnsmasq_interface.c
+++ b/src/dnsmasq_interface.c
@@ -554,9 +554,17 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
        char clientIP[ADDRSTRLEN] = { 0 };
        if(config.edns0_ecs && edns->client_set)
        {
+#if (__GNUC__ > 8)
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wstringop-truncation"
+#endif
                // Use ECS provided client
                strncpy(clientIP, edns->client, ADDRSTRLEN);
                clientIP[ADDRSTRLEN-1] = '\0';
+#if (__GNUC__ > 8)
+# pragma GCC diagnostic pop
+#endif
        }
        else
        {

Builds fine for me like this :smile:

DL6ER commented 3 years ago

Well, this is a bit too dirty for me... I'll rather do

 // Use ECS provided client
-strncpy(clientIP, edns->client, ADDRSTRLEN);
+strncpy(clientIP, edns->client, ADDRSTRLEN-1);
 clientIP[ADDRSTRLEN-1] = '\0';

what does the same thing, basically...

edit Or, even better

-char clientIP[ADDRSTRLEN] = { 0 };
+char clientIP[ADDRSTRLEN+1] = { 0 };
 // Use ECS provided client
 strncpy(clientIP, edns->client, ADDRSTRLEN);
-clientIP[ADDRSTRLEN-1] = '\0';
+clientIP[ADDRSTRLEN] = '\0';
ganto commented 3 years ago

That's definitely prettier and compiles fine :+1: