Sanitizer reported memory leak for '--invalid' option or port number is missed cases to redis-server. by hwware · Pull Request #12322 · redis/redis

hwware · 2023-06-15T18:15:06Z

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

madolson · 2023-06-15T20:36:20Z

src/config.c

+            sdsfreesplitres(argv,argc);
+            goto loaderr;


Aren't every other goto loaderr a problem? I would rather us add a check in loaderr saying if (argv) sdsfreesplitres(argv,argc); Then add all of these on every line.

Hi @madolson ,
I agree with you, I have already started checking for other cases. I will add as you mentioned and test.

can't we put the cleanup in the loaderr label, and nullify something in the (i assume fewer) other places that we call sdsfreesplitres). this will result in fewer changes, and safer code.

Hi @oranagra ,

argv and argc are declared in for loop so if we try to free in loaderr, pointer and variable is going out of scope. In order to do we have to bring argv and argc to function scope. I feel its safe to bring them, I will change scope and test all the cases, if everything works fine will update the diff. Thanks!

We can move the declaration up outside of the loop.

hwware · 2023-06-15T21:26:19Z

Hi @madolson ,
Updated for all other checks and tested. Could you please have a look.

oranagra · 2023-06-16T12:51:29Z

just to make sure, this isn't really a leak (the process is being terminated), it's just done to keep things clean, and keep valgrind happy, right?
please make it clear in the top comment, if that's right. (if not, then it should be on the release notes)

hwware · 2023-06-16T19:11:59Z

Yes, you're right @oranagra, it is related to clean up missing prior process is being terminated, updated the top summary and also updated the diff. Thanks!

src/config.c

Invoke sdsfreesplitres prior exiting for negative options

cd6c268

hwware requested a review from oranagra June 15, 2023 18:29

madolson reviewed Jun 15, 2023

View reviewed changes

update the free for all other checks

2b70bb3

hwware requested a review from madolson June 15, 2023 21:26

hwware changed the title ~~Memory leak issue when invalid as option or port number is not given along with port option to redis-server.~~ Sanitizer reported memory leak for '--invalid' option or port number is missed cases to redis-server. Jun 16, 2023

hwware added 2 commits June 16, 2023 18:48

changed the variable scope and added condition in loaderr

ae4eec4

corrections

a40bc16

oranagra reviewed Jun 18, 2023

View reviewed changes

src/config.c Show resolved Hide resolved

nullify after free other than loaderr

ab33b5a

hwware requested a review from oranagra June 19, 2023 22:50

oranagra approved these changes Jun 20, 2023

View reviewed changes

oranagra merged commit 813924b into redis:unstable Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitizer reported memory leak for '--invalid' option or port number is missed cases to redis-server.#12322

Sanitizer reported memory leak for '--invalid' option or port number is missed cases to redis-server.#12322
oranagra merged 5 commits intoredis:unstablefrom
hwware:mem-leak-issue

hwware commented Jun 15, 2023 •

edited

Loading

Uh oh!

madolson Jun 15, 2023

Uh oh!

hwware Jun 15, 2023

Uh oh!

oranagra Jun 16, 2023

Uh oh!

hwware Jun 16, 2023

Uh oh!

madolson Jun 16, 2023

Uh oh!

hwware commented Jun 15, 2023

Uh oh!

oranagra commented Jun 16, 2023

Uh oh!

hwware commented Jun 16, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hwware commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

hwware Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

hwware Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

madolson Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

hwware commented Jun 15, 2023

Uh oh!

oranagra commented Jun 16, 2023

Uh oh!

hwware commented Jun 16, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hwware commented Jun 15, 2023 •

edited

Loading