From 42c8c61813a8c3287b8acb3155473551a491aa2b Mon Sep 17 00:00:00 2001 From: sundb Date: Tue, 18 Apr 2023 14:53:51 +0800 Subject: Fix some compile warnings and errors when building with gcc-12 or clang (#12035) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR is to fix the compilation warnings and errors generated by the latest complier toolchain, and to add a new runner of the latest toolchain for daily CI. ## Fix various compilation warnings and errors 1) jemalloc.c COMPILER: clang-14 with FORTIFY_SOURCE WARNING: ``` src/jemalloc.c:1028:7: warning: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma? [-Wstring-concatenation] "/etc/malloc.conf", ^ src/jemalloc.c:1027:3: note: place parentheses around the string literal to silence warning "\"name\" of the file referenced by the symbolic link named " ^ ``` REASON: the compiler to alert developers to potential issues with string concatenation that may miss a comma, just like #9534 which misses a comma. SOLUTION: use `()` to tell the compiler that these two line strings are continuous. 2) config.h COMPILER: clang-14 with FORTIFY_SOURCE WARNING: ``` In file included from quicklist.c:36: ./config.h:319:76: warning: attribute declaration must precede definition [-Wignored-attributes] char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead"))); ``` REASON: Enabling _FORTIFY_SOURCE will cause the compiler to use `strcpy()` with check, it results in a deprecated attribute declaration after including . SOLUTION: move the deprecated attribute declaration from config.h to fmacro.h before "#include ". 3) networking.c COMPILER: GCC-12 WARNING: ``` networking.c: In function ‘addReplyDouble.part.0’: networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 876 | dbuf[start] = '$'; | ^ networking.c:868:14: note: at offset -5 into destination object ‘dbuf’ of size 5152 868 | char dbuf[MAX_LONG_DOUBLE_CHARS+32]; | ^ networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 876 | dbuf[start] = '$'; | ^ networking.c:868:14: note: at offset -6 into destination object ‘dbuf’ of size 5152 868 | char dbuf[MAX_LONG_DOUBLE_CHARS+32]; ``` REASON: GCC-12 predicts that digits10() may return 9 or 10 through `return 9 + (v >= 1000000000UL)`. SOLUTION: add an assert to let the compiler know the possible length; 4) redis-cli.c & redis-benchmark.c COMPILER: clang-14 with FORTIFY_SOURCE WARNING: ``` redis-benchmark.c:1621:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL redis-cli.c:3015:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL ``` REASON: when _FORTIFY_SOURCE is enabled, the compiler will use the print() with check, which is a macro. this may result in the use of directives within the macro, which is undefined behavior. SOLUTION: move the directives-related code out of `print()`. 5) server.c COMPILER: gcc-13 with FORTIFY_SOURCE WARNING: ``` In function 'lookupCommandLogic', inlined from 'lookupCommandBySdsLogic' at server.c:3139:32: server.c:3102:66: error: '*(robj **)argv' may be used uninitialized [-Werror=maybe-uninitialized] 3102 | struct redisCommand *base_cmd = dictFetchValue(commands, argv[0]->ptr); | ~~~~^~~ ``` REASON: The compiler thinks that the `argc` returned by `sdssplitlen()` could be 0, resulting in an empty array of size 0 being passed to lookupCommandLogic. this should be a false positive, `argc` can't be 0 when strings are not NULL. SOLUTION: add an assert to let the compiler know that `argc` is positive. 6) sha1.c COMPILER: gcc-12 WARNING: ``` In function ‘SHA1Update’, inlined from ‘SHA1Final’ at sha1.c:195:5: sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread] 152 | SHA1Transform(context->state, &data[i]); | ^ sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’ sha1.c: In function ‘SHA1Final’: sha1.c:56:6: note: in a call to function ‘SHA1Transform’ 56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64]) | ^ In function ‘SHA1Update’, inlined from ‘SHA1Final’ at sha1.c:198:9: sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread] 152 | SHA1Transform(context->state, &data[i]); | ^ sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’ sha1.c: In function ‘SHA1Final’: sha1.c:56:6: note: in a call to function ‘SHA1Transform’ 56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64]) ``` REASON: due to the bug[https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922], when enable LTO, gcc-12 will not see `diagnostic ignored "-Wstringop-overread"`, resulting in a warning. SOLUTION: temporarily set SHA1Update to noinline to avoid compiler warnings due to LTO being enabled until the above gcc bug is fixed. 7) zmalloc.h COMPILER: GCC-12 WARNING: ``` In function ‘memset’, inlined from ‘moduleCreateContext’ at module.c:877:5, inlined from ‘RM_GetDetachedThreadSafeContext’ at module.c:8410:5: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: warning: ‘__builtin_memset’ writing 104 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] 59 | return __builtin___memset_chk (__dest, __ch, __len, ``` REASON: due to the GCC-12 bug [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503], GCC-12 cannot see alloc_size, which causes GCC to think that the actual size of memory is 0 when checking with __glibc_objsize0(). SOLUTION: temporarily set malloc-related interfaces to `noinline` to avoid compiler warnings due to LTO being enabled until the above gcc bug is fixed. ## Other changes 1) Fixed `ps -p [pid]` doesn't output `` when using procps 4.x causing `replication child dies when parent is killed - diskless` test to fail. 2) Add a new fortify CI with GCC-13 and ubuntu-lunar docker image. --- .github/workflows/daily.yml | 46 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) (limited to '.github') diff --git a/.github/workflows/daily.yml b/.github/workflows/daily.yml index 7685ca1a0..42a13f100 100644 --- a/.github/workflows/daily.yml +++ b/.github/workflows/daily.yml @@ -11,7 +11,7 @@ on: inputs: skipjobs: description: 'jobs to skip (delete the ones you wanna keep, do not leave empty)' - default: 'valgrind,sanitizer,tls,freebsd,macos,alpine,32bit,iothreads,ubuntu,centos,malloc,specific,reply-schema' + default: 'valgrind,sanitizer,tls,freebsd,macos,alpine,32bit,iothreads,ubuntu,centos,malloc,specific,fortify,reply-schema' skiptests: description: 'tests to skip (delete the ones you wanna keep, do not leave empty)' default: 'redis,modules,sentinel,cluster,unittest' @@ -71,6 +71,50 @@ jobs: if: true && !contains(github.event.inputs.skiptests, 'unittest') run: ./src/redis-server test all --accurate + test-ubuntu-jemalloc-fortify: + runs-on: ubuntu-latest + if: | + (github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) && + !contains(github.event.inputs.skipjobs, 'fortify') + container: ubuntu:lunar + timeout-minutes: 14400 + steps: + - name: prep + if: github.event_name == 'workflow_dispatch' + run: | + echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV + echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV + echo "skipjobs: ${{github.event.inputs.skipjobs}}" + echo "skiptests: ${{github.event.inputs.skiptests}}" + echo "test_args: ${{github.event.inputs.test_args}}" + echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" + - uses: actions/checkout@v3 + with: + repository: ${{ env.GITHUB_REPOSITORY }} + ref: ${{ env.GITHUB_HEAD_REF }} + - name: make + run: | + apt-get update && apt-get install -y make gcc-13 + update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-13 100 + make CC=gcc REDIS_CFLAGS='-Werror -DREDIS_TEST -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3' + - name: testprep + run: apt-get install -y tcl8.6 tclx procps + - name: test + if: true && !contains(github.event.inputs.skiptests, 'redis') + run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}} + - name: module api test + if: true && !contains(github.event.inputs.skiptests, 'modules') + run: ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}} + - name: sentinel tests + if: true && !contains(github.event.inputs.skiptests, 'sentinel') + run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} + - name: cluster tests + if: true && !contains(github.event.inputs.skiptests, 'cluster') + run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}} + - name: unittest + if: true && !contains(github.event.inputs.skiptests, 'unittest') + run: ./src/redis-server test all --accurate + test-ubuntu-libc-malloc: runs-on: ubuntu-latest if: | -- cgit v1.2.1