Fix some compile warnings and errors when building with gcc-12 or clang (#12035)

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 <features.h>.

SOLUTION: move the deprecated attribute declaration from config.h to fmacro.h before "#include <features.h>".

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 `<defunct>` 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.
This commit is contained in:
sundb 2023-04-18 14:53:51 +08:00 committed by GitHub
parent d2db4aa753
commit 42c8c61813
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 128 additions and 66 deletions

View File

@ -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: |

View File

@ -1024,8 +1024,8 @@ malloc_conf_init_helper(sc_data_t *sc_data, unsigned bin_shard_sizes[SC_NBINS],
static const char *opts_explain[MALLOC_CONF_NSOURCES] = {
"string specified via --with-malloc-conf",
"string pointed to by the global variable malloc_conf",
"\"name\" of the file referenced by the symbolic link named "
"/etc/malloc.conf",
("\"name\" of the file referenced by the symbolic link named "
"/etc/malloc.conf"),
"value of the environment variable MALLOC_CONF"
};
unsigned i;

View File

@ -309,16 +309,6 @@ int pthread_setname_np(const char *name);
void setcpuaffinity(const char *cpulist);
#endif
/* deprecate unsafe functions
*
* NOTE: We do not use the poison pragma since it
* will error on stdlib definitions in files as well*/
#if (__GNUC__ && __GNUC__ >= 4) && !defined __APPLE__
int sprintf(char *str, const char *format, ...) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of snprintf instead")));
char *strcpy(char *restrict dest, const char *src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcpy instead")));
char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead")));
#endif
/* Test for posix_fadvise() */
#if defined(__linux__) || __FreeBSD__ >= 10
#define HAVE_FADVISE

View File

@ -58,6 +58,16 @@
#define _LARGEFILE_SOURCE
#define _FILE_OFFSET_BITS 64
/* deprecate unsafe functions
*
* NOTE: We do not use the poison pragma since it
* will error on stdlib definitions in files as well*/
#if (__GNUC__ && __GNUC__ >= 4) && !defined __APPLE__
int sprintf(char *str, const char *format, ...) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of snprintf instead")));
char *strcpy(char *restrict dest, const char *src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcpy instead")));
char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead")));
#endif
#ifdef __linux__
/* features.h uses the defines above to set feature specific defines. */
#include <features.h>

View File

@ -873,6 +873,7 @@ void addReplyDouble(client *c, double d) {
const int dlen = d2string(dbuf+7,sizeof(dbuf)-7,d);
int digits = digits10(dlen);
int start = 4 - digits;
serverAssert(start >= 0);
dbuf[start] = '$';
/* Convert `dlen` to string, putting it's digits after '$' and before the

View File

@ -1414,6 +1414,7 @@ int parseOptions(int argc, char **argv) {
int i;
int lastarg;
int exit_status = 1;
char *tls_usage;
for (i = 1; i < argc; i++) {
lastarg = (i == (argc-1));
@ -1575,8 +1576,31 @@ invalid:
printf("Invalid option \"%s\" or option argument missing\n\n",argv[i]);
usage:
tls_usage =
#ifdef USE_OPENSSL
" --tls Establish a secure TLS connection.\n"
" --sni <host> Server name indication for TLS.\n"
" --cacert <file> CA Certificate file to verify with.\n"
" --cacertdir <dir> Directory where trusted CA certificates are stored.\n"
" If neither cacert nor cacertdir are specified, the default\n"
" system-wide trusted root certs configuration will apply.\n"
" --insecure Allow insecure TLS connection by skipping cert validation.\n"
" --cert <file> Client certificate to authenticate with.\n"
" --key <file> Private key file to authenticate with.\n"
" --tls-ciphers <list> Sets the list of preferred ciphers (TLSv1.2 and below)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string.\n"
#ifdef TLS1_3_VERSION
" --tls-ciphersuites <list> Sets the list of preferred ciphersuites (TLSv1.3)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string,\n"
" and specifically for TLSv1.3 ciphersuites.\n"
#endif
#endif
"";
printf(
"%s%s", /* Split to avoid strings longer than 4095 (-Woverlength-strings). */
"%s%s%s", /* Split to avoid strings longer than 4095 (-Woverlength-strings). */
"Usage: redis-benchmark [OPTIONS] [COMMAND ARGS...]\n\n"
"Options:\n"
" -h <hostname> Server hostname (default 127.0.0.1)\n"
@ -1617,29 +1641,10 @@ usage:
" The -t option is ignored if a specific command is supplied\n"
" on the command line.\n"
" -I Idle mode. Just open N idle connections and wait.\n"
" -x Read last argument from STDIN.\n"
#ifdef USE_OPENSSL
" --tls Establish a secure TLS connection.\n"
" --sni <host> Server name indication for TLS.\n"
" --cacert <file> CA Certificate file to verify with.\n"
" --cacertdir <dir> Directory where trusted CA certificates are stored.\n"
" If neither cacert nor cacertdir are specified, the default\n"
" system-wide trusted root certs configuration will apply.\n"
" --insecure Allow insecure TLS connection by skipping cert validation.\n"
" --cert <file> Client certificate to authenticate with.\n"
" --key <file> Private key file to authenticate with.\n"
" --tls-ciphers <list> Sets the list of preferred ciphers (TLSv1.2 and below)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string.\n"
#ifdef TLS1_3_VERSION
" --tls-ciphersuites <list> Sets the list of preferred ciphersuites (TLSv1.3)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string,\n"
" and specifically for TLSv1.3 ciphersuites.\n"
#endif
#endif
" -x Read last argument from STDIN.\n",
tls_usage,
" --help Output this help and exit.\n"
" --version Output version and exit.\n\n",
" --version Output version and exit.\n\n"
"Examples:\n\n"
" Run the benchmark with the default configuration against 127.0.0.1:6379:\n"
" $ redis-benchmark\n\n"

View File

@ -2981,6 +2981,29 @@ static void parseEnv() {
static void usage(int err) {
sds version = cliVersion();
FILE *target = err ? stderr: stdout;
const char *tls_usage =
#ifdef USE_OPENSSL
" --tls Establish a secure TLS connection.\n"
" --sni <host> Server name indication for TLS.\n"
" --cacert <file> CA Certificate file to verify with.\n"
" --cacertdir <dir> Directory where trusted CA certificates are stored.\n"
" If neither cacert nor cacertdir are specified, the default\n"
" system-wide trusted root certs configuration will apply.\n"
" --insecure Allow insecure TLS connection by skipping cert validation.\n"
" --cert <file> Client certificate to authenticate with.\n"
" --key <file> Private key file to authenticate with.\n"
" --tls-ciphers <list> Sets the list of preferred ciphers (TLSv1.2 and below)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string.\n"
#ifdef TLS1_3_VERSION
" --tls-ciphersuites <list> Sets the list of preferred ciphersuites (TLSv1.3)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string,\n"
" and specifically for TLSv1.3 ciphersuites.\n"
#endif
#endif
"";
fprintf(target,
"redis-cli %s\n"
"\n"
@ -3012,26 +3035,7 @@ static void usage(int err) {
" -D <delimiter> Delimiter between responses for raw formatting (default: \\n).\n"
" -c Enable cluster mode (follow -ASK and -MOVED redirections).\n"
" -e Return exit error code when command execution fails.\n"
#ifdef USE_OPENSSL
" --tls Establish a secure TLS connection.\n"
" --sni <host> Server name indication for TLS.\n"
" --cacert <file> CA Certificate file to verify with.\n"
" --cacertdir <dir> Directory where trusted CA certificates are stored.\n"
" If neither cacert nor cacertdir are specified, the default\n"
" system-wide trusted root certs configuration will apply.\n"
" --insecure Allow insecure TLS connection by skipping cert validation.\n"
" --cert <file> Client certificate to authenticate with.\n"
" --key <file> Private key file to authenticate with.\n"
" --tls-ciphers <list> Sets the list of preferred ciphers (TLSv1.2 and below)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string.\n"
#ifdef TLS1_3_VERSION
" --tls-ciphersuites <list> Sets the list of preferred ciphersuites (TLSv1.3)\n"
" in order of preference from highest to lowest separated by colon (\":\").\n"
" See the ciphers(1ssl) manpage for more information about the syntax of this string,\n"
" and specifically for TLSv1.3 ciphersuites.\n"
#endif
#endif
"%s"
" --raw Use raw formatting for replies (default when STDOUT is\n"
" not a tty).\n"
" --no-raw Force formatted output even when STDOUT is not a tty.\n"
@ -3041,7 +3045,8 @@ static void usage(int err) {
" --quoted-json Same as --json, but produce ASCII-safe quoted strings, not Unicode.\n"
" --show-pushes <yn> Whether to print RESP3 PUSH messages. Enabled by default when\n"
" STDOUT is a tty but can be overridden with --show-pushes no.\n"
" --stat Print rolling stats about server: mem, clients, ...\n",version);
" --stat Print rolling stats about server: mem, clients, ...\n",
version,tls_usage);
fprintf(target,
" --latency Enter a special mode continuously sampling latency.\n"

View File

@ -3128,6 +3128,7 @@ struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s) {
return NULL;
}
serverAssert(argc > 0); /* Avoid warning `-Wmaybe-uninitialized` in lookupCommandLogic() */
robj objects[argc];
robj *argv[argc];
for (j = 0; j < argc; j++) {

View File

@ -15,7 +15,10 @@ typedef struct {
void SHA1Transform(uint32_t state[5], const unsigned char buffer[64]);
void SHA1Init(SHA1_CTX* context);
void SHA1Update(SHA1_CTX* context, const unsigned char* data, uint32_t len);
/* 'noinline' attribute is intended to prevent the `-Wstringop-overread` warning
* when using gcc-12 later with LTO enabled. It may be removed once the
* bug[https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922] is fixed. */
__attribute__((noinline)) void SHA1Update(SHA1_CTX* context, const unsigned char* data, uint32_t len);
void SHA1Final(unsigned char digest[20], SHA1_CTX* context);
#ifdef REDIS_TEST

View File

@ -96,13 +96,16 @@
#define HAVE_DEFRAG
#endif
__attribute__((malloc,alloc_size(1))) void *zmalloc(size_t size);
__attribute__((malloc,alloc_size(1))) void *zcalloc(size_t size);
__attribute__((malloc,alloc_size(1,2))) void *zcalloc_num(size_t num, size_t size);
__attribute__((alloc_size(2))) void *zrealloc(void *ptr, size_t size);
__attribute__((malloc,alloc_size(1))) void *ztrymalloc(size_t size);
__attribute__((malloc,alloc_size(1))) void *ztrycalloc(size_t size);
__attribute__((alloc_size(2))) void *ztryrealloc(void *ptr, size_t size);
/* 'noinline' attribute is intended to prevent the `-Wstringop-overread` warning
* when using gcc-12 later with LTO enabled. It may be removed once the
* bug[https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503] is fixed. */
__attribute__((malloc,alloc_size(1),noinline)) void *zmalloc(size_t size);
__attribute__((malloc,alloc_size(1),noinline)) void *zcalloc(size_t size);
__attribute__((malloc,alloc_size(1,2),noinline)) void *zcalloc_num(size_t num, size_t size);
__attribute__((alloc_size(2),noinline)) void *zrealloc(void *ptr, size_t size);
__attribute__((malloc,alloc_size(1),noinline)) void *ztrymalloc(size_t size);
__attribute__((malloc,alloc_size(1),noinline)) void *ztrycalloc(size_t size);
__attribute__((alloc_size(2),noinline)) void *ztryrealloc(void *ptr, size_t size);
void zfree(void *ptr);
void *zmalloc_usable(size_t size, size_t *usable);
void *zcalloc_usable(size_t size, size_t *usable);

View File

@ -637,7 +637,7 @@ proc get_child_pid {idx} {
}
proc process_is_alive pid {
if {[catch {exec ps -p $pid} err]} {
if {[catch {exec ps -p $pid -f} err]} {
return 0
} else {
if {[string match "*<defunct>*" $err]} { return 0 }