jvoisin / fortify-headers

Standalone portable header-based implementation of FORTIFY_SOURCE=3
BSD Zero Clause License
19 stars 3 forks source link

kernel fails to build on alpine linux with fortify-headers-2.3.1 #62

Open ncopa opened 4 months ago

ncopa commented 4 months ago
In file included from exec-cmd.c:9:
In function 'vsnprintf',
    inlined from 'report.constprop' at subcmd-util.h:13:2:
/usr/include/fortify/stdio.h:162:16: error: 'msg' may be used uninitialized [-Werror=maybe-uninitialized]
  162 |         return __orig_vsnprintf(__s, __n, __f, __v);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/fortify/strings.h:23,
                 from /usr/include/string.h:59,
                 from /usr/include/fortify/string.h:23,
                 from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/include/linux/string.h:6,
                 from exec-cmd.c:3:
/usr/include/fortify/stdio.h: In function 'report.constprop':
/usr/include/fortify/stdio.h:152:1: note: in a call to '__orig_vsnprintf' declared with attribute 'access (read_write, 1, 2)' here
  152 | _FORTIFY_FN(vsnprintf) int vsnprintf(char * _FORTIFY_POS0 __s, size_t __n,
      | ^~~~~~~~~~~
In file included from exec-cmd.c:10:
subcmd-util.h:12:14: note: 'msg' declared here
   12 |         char msg[1024];
      |              ^~~
cc1: all warnings being treated as errors
make[5]: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/build/Makefile.build:98: /home/ncopa/aports/main/linux-lts/src/build-virt.x86_64/tools/objtool/libsubcmd/exec-cmd.o] Error 1
make[4]: *** [Makefile:80: /home/ncopa/aports/main/linux-lts/src/build-virt.x86_64/tools/objtool/libsubcmd/libsubcmd-in.o] Error 2
make[3]: *** [Makefile:78: /home/ncopa/aports/main/linux-lts/src/build-virt.x86_64/tools/objtool/libsubcmd/libsubcmd.a] Error 2
make[2]: *** [Makefile:73: objtool] Error 2
make[1]: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/Makefile:1362: tools/objtool] Error 2
make: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/Makefile:234: __sub-make] Error 2
ncopa commented 4 months ago

This is the code it complains about:

static inline void report(const char *prefix, const char *err, va_list params)
{
    char msg[1024];
    vsnprintf(msg, sizeof(msg), err, params);
    fprintf(stderr, " %s%s\n", prefix, msg);
}

So compiler complains that msg may be uninitialized when initializing it.

Changing it to:

    char msg[1024] = "";

makes it pass, but I don't think that is a good solution.

jvoisin commented 4 months ago

Good catch, this is indeed a copy-pasta typo. It should be fixed by 6f5423255b6d78b0d6979e6319642ae530f3e2b7

ncopa commented 4 months ago

Seems to solve it. Thank you for a super fast fix!

jvoisin commented 4 months ago

Thank you for making Alpine Linux a willing guinea pig :)

ncopa commented 4 months ago

Seems to also affect snprintf:

  DESCEND objtool
In file included from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/include/linux/string.h:6,
                 from parse-options.c:3:
In function 'strncpy',
    inlined from 'get_value' at parse-options.c:138:4:
/usr/include/fortify/string.h:327:16: error: '__orig_strncpy' reading 128 bytes from a region of size 17 [-Werror=stringop-overread]
  327 |         return __orig_strncpy(__d, __s, __n);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/fortify/strings.h:23,
                 from /usr/include/string.h:59,
                 from /usr/include/fortify/string.h:23:
/usr/include/fortify/string.h: In function 'get_value':
/usr/include/fortify/string.h:311:1: note: in a call to function '__orig_strncpy' declared with attribute 'access (read_only, 2, 3)'
  311 | _FORTIFY_FN(strncpy) char *strncpy(char * _FORTIFY_POS0 __d,
      | ^~~~~~~~~~~
In file included from parse-options.c:5:
In function 'snprintf',
    inlined from 'get_value' at parse-options.c:89:5:
/usr/include/fortify/stdio.h:284:16: error: 'msg' may be used uninitialized [-Werror=maybe-uninitialized]
  284 |         return __orig_snprintf(__s, __n, __f, __builtin_va_arg_pack());
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'get_value':
/usr/include/fortify/stdio.h:274:1: note: in a call to '__orig_snprintf' declared with attribute 'access (read_write, 1, 2)' here
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
parse-options.c:85:30: note: 'msg' declared here
   85 |                         char msg[128];
      |                              ^~~
In function 'snprintf',
    inlined from 'get_value' at parse-options.c:92:5:
/usr/include/fortify/stdio.h:284:16: error: 'msg' may be used uninitialized [-Werror=maybe-uninitialized]
  284 |         return __orig_snprintf(__s, __n, __f, __builtin_va_arg_pack());
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'get_value':
/usr/include/fortify/stdio.h:274:1: note: in a call to '__orig_snprintf' declared with attribute 'access (read_write, 1, 2)' here
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
parse-options.c:85:30: note: 'msg' declared here
   85 |                         char msg[128];
      |                              ^~~
In function 'snprintf',
    inlined from 'get_value' at parse-options.c:130:9:
/usr/include/fortify/stdio.h:284:16: error: 'reason' may be used uninitialized [-Werror=maybe-uninitialized]
  284 |         return __orig_snprintf(__s, __n, __f, __builtin_va_arg_pack());
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'get_value':
/usr/include/fortify/stdio.h:274:1: note: in a call to '__orig_snprintf' declared with attribute 'access (read_write, 1, 2)' here
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
parse-options.c:127:22: note: 'reason' declared here
  127 |                 char reason[128];
      |                      ^~~~~~
jvoisin commented 4 months ago

Sigh, I should have been more thorough. Fixed by 9014b0266147dbb74d5d9e6e2c24ae9d21ad7e07

ncopa commented 4 months ago

Not sure if the strncpy should be reported separately? Seems to be another bug.

  CALL    /home/ncopa/aports/main/linux-lts/src/linux-6.6/scripts/checksyscalls.sh
  DESCEND objtool
In file included from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/include/linux/string.h:6,
                 from parse-options.c:3:
In function 'strncpy',
    inlined from 'get_value' at parse-options.c:138:4:
/usr/include/fortify/string.h:327:16: error: '__orig_strncpy' reading 128 bytes from a region of size 17 [-Werror=stringop-overread]
  327 |         return __orig_strncpy(__d, __s, __n);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/fortify/strings.h:23,
                 from /usr/include/string.h:59,
                 from /usr/include/fortify/string.h:23:
/usr/include/fortify/string.h: In function 'get_value':
/usr/include/fortify/string.h:311:1: note: in a call to function '__orig_strncpy' declared with attribute 'access (read_only, 2, 3)'
  311 | _FORTIFY_FN(strncpy) char *strncpy(char * _FORTIFY_POS0 __d,
      | ^~~~~~~~~~~
cc1: all warnings being treated as errors

The code it comes from looks legit?

    if (opt->flags & PARSE_OPT_NOBUILD) {
        char reason[128];
        bool noarg = false;

        err = snprintf(reason, sizeof(reason),
                opt->flags & PARSE_OPT_CANSKIP ?
                    "is being ignored because %s " :
                    "is not available because %s",
                opt->build_opt);
        reason[sizeof(reason) - 1] = '\0';

        if (err < 0)
            strncpy(reason, opt->flags & PARSE_OPT_CANSKIP ?
                    "is being ignored" :
                    "is not available",
                    sizeof(reason));
ncopa commented 4 months ago

Maybe something like this? It will not read more than max_len_s:

diff --git a/include/string.h b/include/string.h
index c317b1e..0347ddf 100644
--- a/include/string.h
+++ b/include/string.h
@@ -324,7 +324,7 @@ _FORTIFY_FN(strncpy) char *strncpy(char * _FORTIFY_POS0 __d,
        if (__n > __b)
                __builtin_trap();

-       return __orig_strncpy(__d, __s, __n);
+       return __orig_strncpy(__d, __s, max_len_s);
 #endif
 }
ncopa commented 4 months ago

And with the above fix applied it continues with:

  DESCEND objtool
rm -f /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/libsubcmd/libsubcmd.a && ar rcs /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/libsubcmd/libsubcmd.a /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/libsubcmd/libsubcmd-in.o
make[4]: 'install_headers' is up to date.
In file included from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/include/linux/panic.h:6,
                 from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/include/linux/kernel.h:11,
                 from /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/libsubcmd/include/subcmd/parse-options.h:5,
                 from /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/objtool/include/objtool/builtin.h:8,
                 from check.c:11:
In function 'sprintf',
    inlined from 'offstr' at /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/objtool/include/objtool/warn.h:33:9:
/usr/include/fortify/stdio.h:300:23: error: '__orig_snprintf' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  300 |                 __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/fortify/strings.h:23,
                 from /usr/include/string.h:59,
                 from /usr/include/fortify/string.h:23,
                 from check.c:6:
/usr/include/fortify/stdio.h: In function 'offstr':
/usr/include/fortify/stdio.h:274:1: note: in a call to function '__orig_snprintf' declared with attribute 'access (read_only, 3)'
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
In function 'sprintf',
    inlined from 'offstr' at /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/objtool/include/objtool/warn.h:35:4:
/usr/include/fortify/stdio.h:300:23: error: '__orig_snprintf' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  300 |                 __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'offstr':
/usr/include/fortify/stdio.h:274:1: note: in a call to function '__orig_snprintf' declared with attribute 'access (read_only, 3)'
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
In function 'sprintf',
    inlined from 'offstr' at /home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/objtool/include/objtool/warn.h:38:3:
/usr/include/fortify/stdio.h:300:23: error: '__orig_snprintf' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  300 |                 __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'offstr':
/usr/include/fortify/stdio.h:274:1: note: in a call to function '__orig_snprintf' declared with attribute 'access (read_only, 3)'
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
In function 'sprintf',
    inlined from 'disas_warned_funcs' at check.c:4624:5,
    inlined from 'check' at check.c:4814:3:
/usr/include/fortify/stdio.h:300:23: error: '__orig_snprintf' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  300 |                 __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fortify/stdio.h: In function 'check':
/usr/include/fortify/stdio.h:274:1: note: in a call to function '__orig_snprintf' declared with attribute 'access (read_only, 3)'
  274 | _FORTIFY_FN(snprintf) int snprintf(char *__s, size_t __n,
      | ^~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/tools/build/Makefile.build:98: /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/check.o] Error 1
make[3]: *** [Makefile:66: /home/ncopa/aports/main/linux-lts/src/build-lts.x86_64/tools/objtool/objtool-in.o] Error 2
make[2]: *** [Makefile:73: objtool] Error 2
make[1]: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/Makefile:1362: tools/objtool] Error 2
make: *** [/home/ncopa/aports/main/linux-lts/src/linux-6.6/Makefile:234: __sub-make] Error 2

The offstr looks like:


static inline char *offstr(struct section *sec, unsigned long offset)
{
    bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
    struct symbol *sym = NULL;
    char *str;
    int len;

    if (is_text)
        sym = find_func_containing(sec, offset);
    if (!sym)
        sym = find_symbol_containing(sec, offset);

    if (sym) {
        str = malloc(strlen(sym->name) + strlen(sec->name) + 40);
        len = sprintf(str, "%s+0x%lx", sym->name, offset - sym->offset);
        if (opts.sec_address)
            sprintf(str+len, " (%s+0x%lx)", sec->name, offset);
    } else {
        str = malloc(strlen(sec->name) + 20);
        sprintf(str, "%s+0x%lx", sec->name, offset);
    }

    return str;
}
jvoisin commented 4 months ago

/usr/include/fortify/stdio.h:300:23: error: '__orig_snprintf' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]

Is a bit confusing, since 18446744073709551615 is (size_t)-1 on 64b, and the code is:

  if (__b != (__fh_size_t)-1) {
          __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
ncopa commented 4 months ago

Weird indeed. It also happens with this:

        if (__b != (__fh_size_t)-1 && __b != (__fh_size_t)18446744073709551615) {
                __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
jvoisin commented 4 months ago

#define __fh_size_t __typeof__(sizeof(char)) is size_t on my machine, so I'm really confused :/

ncopa commented 4 months ago

I confirmed it is 8 bytes here, on my x86_64.

markus-oberhumer commented 4 months ago

FYI, the type of sizeof(...) always is size_t.

(Edit: just passing by because our CI build on alpine:edge started to fail because of false fortify positives).

jvoisin commented 4 months ago

So wtf is going on :D

ncopa commented 4 months ago

I have tried this to confirm that it actually is __b.

 __r = __orig_snprintf(__s, 20000, __f, __builtin_va_arg_pack());

It makes it build the objtool.

So it must be __b that is the problem.

Then I tried with:

        if (__b < 9223372036854775807) {
                __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());

But it still triggers the problem. I have no clue what is going on here.

ncopa commented 4 months ago

(Edit: just passing by because our CI build on alpine:edge started to fail because of false fortify positives).

@markus-oberhumer is it the same error, or something else?

I think we should try create a smaller test case where we can debug this.

markus-oberhumer commented 4 months ago

@ncopa Looks like we hit strncpy

https://github.com/upx/upx-test-build-with-zig/actions/runs/9874643415/job/27269509241

ncopa commented 4 months ago

@ncopa Looks like we hit strncpy

https://github.com/upx/upx-test-build-with-zig/actions/runs/9874643415/job/27269509241

I'll push the fix for that, while we scratch our heads.

ncopa commented 4 months ago

This makes build pass:

    if (__b < 1) {
                __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());

This fails with same error:

        if (__b < 2) {
                __r = __orig_snprintf(__s, __b, __f, __builtin_va_arg_pack());
t8m commented 4 months ago

Try building openssl - it will also fail to build with multiple false positive warnings if -Werror is used.

t8m commented 4 months ago

https://github.com/openssl/openssl/actions/runs/9876435764/job/27275535784 https://github.com/openssl/openssl/actions/runs/9868489574/job/27250610791 https://github.com/openssl/openssl/actions/runs/9876734277/job/27276617279

ncopa commented 4 months ago

I have a smallish reproducer:

#include <stdio.h>

static char *offstr(char *str)
{
    int len = 0;

    len = sprintf(str, "%s+0x%lx", "foo", (long unsigned int)0);
    sprintf(str+len, " (%s+0x%lx)","bar", (long unsigned int)0);
    if (len < 0)
        return NULL;
    return str;
}

int main() {
    char buf[100];
    char *c = offstr(buf);
    printf("%s\n", c);
    return 0;
}
ncopa commented 4 months ago

I have reverted the fortify-headers upgrade in Alpine Linux til we have this sorted.

markus-oberhumer commented 3 months ago

@jvoisin Could you please create a single-file reproducer on Compiler Explorer https://godbolt.org/ ?

jvoisin commented 2 months ago

f2e7f24daaa43c0927130b6ed02c3ed17689b3ca should work around the issue, as I really don't want to dive into gcc's code.