Skip to content

Sentinel: fix a free-after-use issue re-registering Sentinels.#10333

Merged
yossigo merged 4 commits intoredis:unstablefrom
moticless:sentinel-macos-sanitizer-fix
Feb 23, 2022
Merged

Sentinel: fix a free-after-use issue re-registering Sentinels.#10333
yossigo merged 4 commits intoredis:unstablefrom
moticless:sentinel-macos-sanitizer-fix

Conversation

@moticless
Copy link
Collaborator

@moticless moticless commented Feb 23, 2022 •
edited by yossigo
Loading

In case HELLO message received from another sentinel, with same address like another instance registered in the past but with different runid. Then there was cumbersome logic to modify the instance the port to 0 to in order to mark as invalid and later on to delete it. But the deletion is happening during update of instances in such a way that we might end up accessing an instance that was deleted just before.

Didn't find a good reason why to postpone the deletion action of an obsolete instance (deletion is taking place instantly, for other cases ) -> Lets delete at once
There is a mixture of logic of Sentinel address update with the logic of deletion of Sentinels that match a given Address -> Split to two!

@moticless moticless requested review from hwware and yossigo February 23, 2022 09:49
@moticless moticless mentioned this pull request Feb 23, 2022
Closed
@yossigo
Copy link
Collaborator

yossigo commented Feb 23, 2022

@moticless I'm still getting failures:

==2768==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000322b13 at pc 0x000108384c00 bp 0x7ffee7f4d0a0 sp 0x7ffee7f4c848
READ of size 1 at 0x604000322b13 thread T0
    #0 0x108384bff in wrap_strcmp+0x32f (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x15bff)
    #1 0x107eaf3e5 in removeMatchingSentinelFromMaster sentinel.c:1441
    #2 0x107ebbee6 in sentinelProcessHelloMessage sentinel.c:2886
    #3 0x107ec6a18 in sentinelPublishCommand sentinel.c:4390
    #4 0x107ce6203 in call server.c:3206
    #5 0x107ce9bc0 in processCommand server.c:3806
    #6 0x107d4fc87 in processInputBuffer networking.c:2445
    #7 0x107d3474e in readQueryFromClient networking.c:2557
    #8 0x107f878d5 in connSocketEventHandler connection.c:310
    #9 0x107cc3668 in aeProcessEvents ae.c:436
    #10 0x107cc43bc in aeMain ae.c:496
    #11 0x107cfe46e in main server.c:6960
    #12 0x7fff208dff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

0x604000322b13 is located 3 bytes inside of 44-byte region [0x604000322b10,0x604000322b3c)
freed by thread T0 here:
    #0 0x1083b34e9 in wrap_free+0xa9 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x444e9)
    #1 0x107ea8885 in releaseSentinelRedisInstance sentinel.c:1369
    #2 0x107cca7b4 in dictGenericDelete dict.c:421
    #3 0x107cca2aa in dictDelete dict.c:437
    #4 0x107eaf414 in removeMatchingSentinelFromMaster sentinel.c:1442
    #5 0x107ebbee6 in sentinelProcessHelloMessage sentinel.c:2886
    #6 0x107ec6a18 in sentinelPublishCommand sentinel.c:4390
    #7 0x107ce6203 in call server.c:3206
    #8 0x107ce9bc0 in processCommand server.c:3806
    #9 0x107d4fc87 in processInputBuffer networking.c:2445
    #10 0x107d3474e in readQueryFromClient networking.c:2557
    #11 0x107f878d5 in connSocketEventHandler connection.c:310
    #12 0x107cc3668 in aeProcessEvents ae.c:436
    #13 0x107cc43bc in aeMain ae.c:496
    #14 0x107cfe46e in main server.c:6960
    #15 0x7fff208dff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

previously allocated by thread T0 here:
    #0 0x1083b33a0 in wrap_malloc+0xa0 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x443a0)
    #1 0x107d111a0 in zmalloc_usable zmalloc.c:140
    #2 0x107cff70f in _sdsnewlen sds.c:117
    #3 0x107ebc001 in sentinelProcessHelloMessage sentinel.c:2902
    #4 0x107faa570 in redisProcessCallbacks async.c:572
    #5 0x107cc3668 in aeProcessEvents ae.c:436
    #6 0x107cc43bc in aeMain ae.c:496
    #7 0x107cfe46e in main server.c:6960
    #8 0x7fff208dff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

@moticless
Copy link
Collaborator Author

moticless commented Feb 23, 2022

@yossigo, I fixed a memory issue ... by adding new one :) I slipped in test due to toggling between build layouts... Tested as expected this time. Thanks.

hwware added a commit to hwware/redis that referenced this pull request Feb 23, 2022
@hwware
Update codes
ddccb71 
Follow redis#10333
@yossigo yossigo changed the title Sentinel macOS sanitizer fix (heap-use-after-free) Sentinel: fix a free-after-use issue re-registering Sentinels. Feb 23, 2022
@yossigo yossigo merged commit a7179e7 into redis:unstable Feb 23, 2022
@yossigo yossigo added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 23, 2022
@oranagra oranagra mentioned this pull request Feb 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yossigo yossigo yossigo approved these changes

@hwware hwware Awaiting requested review from hwware

Assignees

No one assigned

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@moticless @yossigo