From 813924b4c36856b2c2ca6f6cc31ecc732ec74f4a Mon Sep 17 00:00:00 2001 From: Wen Hui Date: Tue, 20 Jun 2023 15:07:29 +0800 Subject: [PATCH] Sanitizer reported memory leak for '--invalid' option or port number is missed cases to redis-server. (#12322) Observed that the sanitizer reported memory leak as clean up is not done before the process termination in negative/following cases: **- when we passed '--invalid' as option to redis-server.** ``` -vm:~/mem-leak-issue/redis$ ./src/redis-server --invalid *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 2 >>> 'invalid' Bad directive or wrong number of arguments ================================================================= ==865778==ERROR: LeakSanitizer: detected memory leaks Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7f0985f65867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x558ec86686ec in ztrymalloc_usable_internal /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:117 #2 0x558ec86686ec in ztrymalloc_usable /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:135 #3 0x558ec86686ec in ztryrealloc_usable_internal /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:276 #4 0x558ec86686ec in zrealloc /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:327 #5 0x558ec865dd7e in sdssplitargs /home/ubuntu/mem-leak-issue/redis/src/sds.c:1172 #6 0x558ec87a1be7 in loadServerConfigFromString /home/ubuntu/mem-leak-issue/redis/src/config.c:472 #7 0x558ec87a13b3 in loadServerConfig /home/ubuntu/mem-leak-issue/redis/src/config.c:718 #8 0x558ec85e6f15 in main /home/ubuntu/mem-leak-issue/redis/src/server.c:7258 #9 0x7f09856e5d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s). ``` **- when we pass '--port' as option and missed to add port number to redis-server.** ``` vm:~/mem-leak-issue/redis$ ./src/redis-server --port *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 2 >>> 'port' wrong number of arguments ================================================================= ==865846==ERROR: LeakSanitizer: detected memory leaks Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7fdcdbb1f867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x557e8b04f6ec in ztrymalloc_usable_internal /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:117 #2 0x557e8b04f6ec in ztrymalloc_usable /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:135 #3 0x557e8b04f6ec in ztryrealloc_usable_internal /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:276 #4 0x557e8b04f6ec in zrealloc /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:327 #5 0x557e8b044d7e in sdssplitargs /home/ubuntu/mem-leak-issue/redis/src/sds.c:1172 #6 0x557e8b188be7 in loadServerConfigFromString /home/ubuntu/mem-leak-issue/redis/src/config.c:472 #7 0x557e8b1883b3 in loadServerConfig /home/ubuntu/mem-leak-issue/redis/src/config.c:718 #8 0x557e8afcdf15 in main /home/ubuntu/mem-leak-issue/redis/src/server.c:7258 #9 0x7fdcdb29fd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 Indirect leak of 10 byte(s) in 1 object(s) allocated from: #0 0x7fdcdbb1fc18 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164 #1 0x557e8b04f9aa in ztryrealloc_usable_internal /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:287 #2 0x557e8b04f9aa in ztryrealloc_usable /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:317 #3 0x557e8b04f9aa in zrealloc_usable /home/ubuntu/mem-leak-issue/redis/src/zmalloc.c:342 #4 0x557e8b033f90 in _sdsMakeRoomFor /home/ubuntu/mem-leak-issue/redis/src/sds.c:271 #5 0x557e8b033f90 in sdsMakeRoomFor /home/ubuntu/mem-leak-issue/redis/src/sds.c:295 #6 0x557e8b033f90 in sdscatlen /home/ubuntu/mem-leak-issue/redis/src/sds.c:486 #7 0x557e8b044e1f in sdssplitargs /home/ubuntu/mem-leak-issue/redis/src/sds.c:1165 #8 0x557e8b188be7 in loadServerConfigFromString /home/ubuntu/mem-leak-issue/redis/src/config.c:472 #9 0x557e8b1883b3 in loadServerConfig /home/ubuntu/mem-leak-issue/redis/src/config.c:718 #10 0x557e8afcdf15 in main /home/ubuntu/mem-leak-issue/redis/src/server.c:7258 #11 0x7fdcdb29fd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 SUMMARY: AddressSanitizer: 18 byte(s) leaked in 2 allocation(s). ``` As part analysis found that the sdsfreesplitres is not called when this condition checks are being hit. Output after the fix: ``` vm:~/mem-leak-issue/redis$ ./src/redis-server --invalid *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 2 >>> 'invalid' Bad directive or wrong number of arguments vm:~/mem-leak-issue/redis$ =========================================== vm:~/mem-leak-issue/redis$ ./src/redis-server --jdhg *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 2 >>> 'jdhg' Bad directive or wrong number of arguments --------------------------------------------------------------------------- vm:~/mem-leak-issue/redis$ ./src/redis-server --port *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 2 >>> 'port' wrong number of arguments ``` Co-authored-by: Oran Agra --- src/config.c | 11 ++++++++--- tests/unit/introspection.tcl | 4 ++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/config.c b/src/config.c index aee8a629f1..a61e3a31d3 100644 --- a/src/config.c +++ b/src/config.c @@ -455,14 +455,13 @@ void loadServerConfigFromString(char *config) { const char *err = NULL; int linenum = 0, totlines, i; sds *lines; + sds *argv = NULL; + int argc; reading_config_file = 1; lines = sdssplitlen(config,strlen(config),"\n",1,&totlines); for (i = 0; i < totlines; i++) { - sds *argv; - int argc; - linenum = i+1; lines[i] = sdstrim(lines[i]," \t\r\n"); @@ -479,6 +478,7 @@ void loadServerConfigFromString(char *config) { /* Skip this line if the resulting command vector is empty. */ if (argc == 0) { sdsfreesplitres(argv,argc); + argv = NULL; continue; } sdstolower(argv[0]); @@ -501,6 +501,7 @@ void loadServerConfigFromString(char *config) { int new_argc; new_argv = sdssplitargs(argv[1], &new_argc); if (!config->interface.set(config, new_argv, new_argc, &err)) { + if(new_argv) sdsfreesplitres(new_argv, new_argc); goto loaderr; } sdsfreesplitres(new_argv, new_argc); @@ -512,6 +513,7 @@ void loadServerConfigFromString(char *config) { } sdsfreesplitres(argv,argc); + argv = NULL; continue; } else { int match = 0; @@ -526,6 +528,7 @@ void loadServerConfigFromString(char *config) { } if (match) { sdsfreesplitres(argv,argc); + argv = NULL; continue; } } @@ -592,6 +595,7 @@ void loadServerConfigFromString(char *config) { err = "Bad directive or wrong number of arguments"; goto loaderr; } sdsfreesplitres(argv,argc); + argv = NULL; } if (server.logfile[0] != '\0') { @@ -629,6 +633,7 @@ void loadServerConfigFromString(char *config) { return; loaderr: + if (argv) sdsfreesplitres(argv,argc); fprintf(stderr, "\n*** FATAL CONFIG FILE ERROR (Redis %s) ***\n", REDIS_VERSION); if (i < totlines) { diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl index 6cee1e2e90..8132ee1978 100644 --- a/tests/unit/introspection.tcl +++ b/tests/unit/introspection.tcl @@ -631,6 +631,10 @@ start_server {tags {"introspection"}} { } test {redis-server command line arguments - error cases} { + # Take '--invalid' as the option. + catch {exec src/redis-server --invalid} err + assert_match {*Bad directive or wrong number of arguments*} $err + catch {exec src/redis-server --port} err assert_match {*'port'*wrong number of arguments*} $err