Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

corrupt-dump-fuzzer test, avoid creating junk keys #9302

Merged
merged 1 commit into from Aug 5, 2021

Conversation

oranagra
Copy link
Member

@oranagra oranagra commented Aug 1, 2021

The execution of the RPOPLPUSH command by the fuzzer created junk keys,
that were later being selected by RANDOMKEY and modified.
This also meant that lists were statistically tested more than other
files.

Fix the fuzzer not to pass junk key names to RPOPLPUSH, and add a check
that detects that new keys are not added by the fuzzer to detect future
similar issues.

The execution of the RPOPLPUSH command by the fuzzer created junk keys,
that were later being selected by RANDOMKEY and modified.
This also meant that lists were statistically tested more than other
files.

Fix the fuzzer not to pass junk key names to RPOPLPUSH, and add a check
that detects that new keys are not added by the fuzzer to detect future
similar issues.
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Aug 1, 2021
@oranagra oranagra requested review from sundb and yossigo August 1, 2021 12:20
@sundb
Copy link
Collaborator

sundb commented Aug 2, 2021

@oranagra It seems to be working, I found problems with other data types.

@oranagra
Copy link
Member Author

oranagra commented Aug 2, 2021

@sundb what seems to be working?
did you get a report of unexpected keys extra keys with this PR? (hopefully with the command that may have generated them)

@sundb
Copy link
Collaborator

sundb commented Aug 2, 2021

debug set-skip-checksum-validation 1
restore key 0 "\x02\x81\xC0\x00\x02\x5F\x31\xC0\x02\x09\x00\xB2\x1B\xE5\x17\x2E\x15\xF4\x6C" replace
=== REDIS BUG REPORT START: Cut & paste starting from here ===
705881:M 02 Aug 2021 00:48:01.339 # Redis 255.255.255 crashed by signal: 11, si_code: 128
705881:M 02 Aug 2021 00:48:01.339 # Accessing address: (nil)
705881:M 02 Aug 2021 00:48:01.339 # Crashed running the instruction at: 0x5590213fe021

------ STACK TRACE ------
EIP:
./src/redis-server *:6379(dictAddRaw+0x131)[0x5590213fe021]

Backtrace:
/lib/x86_64-linux-gnu/libpthread.so.0(+0x153c0)[0x7f3c1bac33c0]
./src/redis-server *:6379(dictAddRaw+0x131)[0x5590213fe021]
./src/redis-server *:6379(dictAdd+0x15)[0x5590213fe1f5]
./src/redis-server *:6379(rdbLoadObject+0x3ce)[0x55902143bdce]
./src/redis-server *:6379(restoreCommand+0x28e)[0x55902146ef1e]
./src/redis-server *:6379(call+0xa1)[0x5590214044a1]
./src/redis-server *:6379(processCommand+0x5d3)[0x5590214060a3]
./src/redis-server *:6379(processCommandAndResetClient+0x20)[0x559021420610]
./src/redis-server *:6379(processInputBuffer+0xce)[0x559021422d9e]
./src/redis-server *:6379(+0x10d73c)[0x5590214bc73c]
./src/redis-server *:6379(aeProcessEvents+0x1ea)[0x5590213fc02a]
./src/redis-server *:6379(aeMain+0x1d)[0x5590213fc3ed]
./src/redis-server *:6379(main+0x405)[0x5590213f2fe5]

set OBJ_ENCODING_HT crash.
No wonder all the problems I found before were about list.

@oranagra
Copy link
Member Author

oranagra commented Aug 2, 2021

ohh, you're saying that because this bug (issue with RPOPLPUSH) kept populating more and more lists, statistically we didn't get to test the other types, and now with this fixed we do... and there are more issues to solve..

@oranagra
Copy link
Member Author

oranagra commented Aug 2, 2021

let's avoid merging this until i fix all the issues it reports.. don't wanna start failing the daily CI until i do..

oranagra added a commit that referenced this pull request Aug 5, 2021
Recently we found two issues in the fuzzer tester: #9302 #9285
After fixing them, more problems surfaced and this PR (as well as #9297) aims to fix them.

Here's a list of the fixes
- Prevent an overflow when allocating a dict hashtable
- Prevent OOM when attempting to allocate a huge string
- Prevent a few invalid accesses in listpack
- Improve sanitization of listpack first entry
- Validate integrity of stream consumer groups PEL
- Validate integrity of stream listpack entry IDs
- Validate ziplist tail followed by extra data which start with 0xff

Co-authored-by: sundb <sundbcn@gmail.com>
@oranagra oranagra merged commit 3f3f678 into redis:unstable Aug 5, 2021
@oranagra oranagra deleted the dump-fuzzer-junk-keys branch August 5, 2021 19:57
sundb added a commit to sundb/redis that referenced this pull request Aug 6, 2021
1) Fix CR

2) In resolving conflict, I reverted all corrupt-test tests, as some
were fixed in redis#9302, and Re-add the new issue caused this pr.

3) ziplistValidateIntegrity fails to validate an empty ziplsit because
hash ziplist->listpack conversion is always deep santization, this
should be fixed in redis#9297 but was missed.
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
Recently we found two issues in the fuzzer tester: redis#9302 redis#9285
After fixing them, more problems surfaced and this PR (as well as redis#9297) aims to fix them.

Here's a list of the fixes
- Prevent an overflow when allocating a dict hashtable
- Prevent OOM when attempting to allocate a huge string
- Prevent a few invalid accesses in listpack
- Improve sanitization of listpack first entry
- Validate integrity of stream consumer groups PEL
- Validate integrity of stream listpack entry IDs
- Validate ziplist tail followed by extra data which start with 0xff

Co-authored-by: sundb <sundbcn@gmail.com>
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
The execution of the RPOPLPUSH command by the fuzzer created junk keys,
that were later being selected by RANDOMKEY and modified.
This also meant that lists were statistically tested more than other
files.

Fix the fuzzer not to pass junk key names to RPOPLPUSH, and add a check
that detects that new keys are not added by the fuzzer to detect future
similar issues.
@oranagra oranagra added this to To Do in 6.2 Backport via automation Oct 3, 2021
@oranagra oranagra moved this from To Do to In progress in 6.2 Backport Oct 3, 2021
oranagra added a commit to oranagra/redis that referenced this pull request Oct 4, 2021
Recently we found two issues in the fuzzer tester: redis#9302 redis#9285
After fixing them, more problems surfaced and this PR (as well as redis#9297) aims to fix them.

Here's a list of the fixes
- Prevent an overflow when allocating a dict hashtable
- Prevent OOM when attempting to allocate a huge string
- Prevent a few invalid accesses in listpack
- Improve sanitization of listpack first entry
- Validate integrity of stream consumer groups PEL
- Validate integrity of stream listpack entry IDs
- Validate ziplist tail followed by extra data which start with 0xff

Co-authored-by: sundb <sundbcn@gmail.com>
(cherry picked from commit 0c90370)
oranagra added a commit to oranagra/redis that referenced this pull request Oct 4, 2021
The execution of the RPOPLPUSH command by the fuzzer created junk keys,
that were later being selected by RANDOMKEY and modified.
This also meant that lists were statistically tested more than other
files.

Fix the fuzzer not to pass junk key names to RPOPLPUSH, and add a check
that detects that new keys are not added by the fuzzer to detect future
similar issues.

(cherry picked from commit 3f3f678)
oranagra added a commit that referenced this pull request Oct 4, 2021
Recently we found two issues in the fuzzer tester: #9302 #9285
After fixing them, more problems surfaced and this PR (as well as #9297) aims to fix them.

Here's a list of the fixes
- Prevent an overflow when allocating a dict hashtable
- Prevent OOM when attempting to allocate a huge string
- Prevent a few invalid accesses in listpack
- Improve sanitization of listpack first entry
- Validate integrity of stream consumer groups PEL
- Validate integrity of stream listpack entry IDs
- Validate ziplist tail followed by extra data which start with 0xff

Co-authored-by: sundb <sundbcn@gmail.com>
(cherry picked from commit 0c90370)
oranagra added a commit that referenced this pull request Oct 4, 2021
The execution of the RPOPLPUSH command by the fuzzer created junk keys,
that were later being selected by RANDOMKEY and modified.
This also meant that lists were statistically tested more than other
files.

Fix the fuzzer not to pass junk key names to RPOPLPUSH, and add a check
that detects that new keys are not added by the fuzzer to detect future
similar issues.

(cherry picked from commit 3f3f678)
@oranagra oranagra moved this from In progress to Done in 6.2 Backport Oct 4, 2021
@enjoy-binbin
Copy link
Collaborator

my daily report this: https://github.com/enjoy-binbin/redis/actions/runs/5634670831/job/15264829706#step:6:3869

it is odd that keys * output is empty:

unexpected keys
keys: 
commands leading to it:
HLEN _hash 
HEXISTS _hash 904177623 
HSCAN _hash -882 
HGET _hash -2654025464 
HRANDFIELD _hash 
HLEN _hash 
HKEYS _hash 
HSCAN _hash -1835358613 
HINCRBY _hash 011120423440421040312042241224312424000201420001132213311421330311021320031223411033024204441413021321221322031243222303324311144430041301120110342431214300344200220041411002143131321133231132 -842681562454 
HGET _hash 133240343401112011044130321440001301434322033201324322403042121344242044243421213012233330224203402332143033031120021234201144104111112412020222204101443013342213434340120040101034102203232144 
HMSET _hash 552 566 
HMSET _hash 505 "\xED\xB1\xCC\x41\x6E\x04\x00\xF3\xBC\x66\xA4\x6F\x07\x46\x78\x84\xF5\x4E\x59\x64\x38\xF2\x7C\x22\x0C\xD5\x3E\x6D\xD7\xF9\xE4\x1A\x8B\x55\x2F\x83\x3E\x2C\x5B\x94\xBD\x8C\x21\x93\xC3\xAB\x1E\x77\xEA\x5B\x28\x63\x57\x82\xF2\xF9\x75\xB8\xC6\xEA\xE8\x59\x27\x04\xEA\xCA\x8B\xF9\x21\x29\xD5\x51\x78\xF5\x2A\x4C\x71\x3F\x7C\x93\xF2\xE9\x1A\x7B\x0E\xFD\xA3\xE1\x70\x65\x7E\x7B\x32\xD9\x2C\x14\x5F\x1C\x02\xBE\x48\x23\x91\x2E\x5D\xF6\xF1\xAA\x5C\x43\x08\x1B\xF0\x74\x39\x22\x07\x27\xFA\xA5\x9D\x61\xB9\x32\x52\x86\xD0\x2F\x41\x27\xEC\x1D\x28\xC0\x69\xFF\x03\x0A\x69\xE7\x24\xB7\x2C\x01\x1A\x3F\x27\xC7\x1E\x7D\x32\x9B\xBD\x7A\xBA" 
HDEL _hash -1602903218 
HVALS _hash 
HLEN _hash 
HSTRLEN _hash 2921022310 
HEXISTS _hash 2001314352 
HLEN _hash 
HRANDFIELD _hash 
HSET _hash 445374049453 447204750705 
HINCRBY _hash -1640349081 389377003716 
HEXISTS _hash 89201243170 
HKEYS _hash 
HSET _hash -15024646192 -980834101317 
HRANDFIELD _hash 
HEXISTS _hash 401637864486 
HSETNX _hash 438 26 
HSETNX _hash "\x93\xD7\xB9\xE8\xF0\x54\xF5\x25\x46\x78\x9B\x2C\xF9\x0B\x88\xFE\x9C\xB3\x9A\xCF\x5A\x20\x64\x3D\x23\x85\xB5\x6E\x82\x58\x60\x3C\xF1\x20\xE8\x75\xE0\x0D\xAB\x05\x2C\xE9\x50\xA5\xF6\x5D\x04\x55\x8A\x51\xB7\x14\x12\x12\x77\xD9\x43\xE0\xDA\xA9\x44\x97\xBA\xF4\xF2\x27\x5D\x31\x31\xA1\xDC\x6B\x52\x01\x0F\xEF\xCE\xDF\x8F\x0E\x70\xD2\x09\x83\xD1\x66\x64\xC3\xCB\x92\xCC\xF3\x1B\x59\x31\x58\x2F\x57\xC5\x90\xAB\x8A\x6A\x59\x0F\xC7\x6B\xF0\x25\x2D\x0E\x97\xA2\x91\x5D\x33\x43\x8C\xB3\xAB\x61\x1D\xEB\xC3\xBC\xBE\x64\xD7\x75\xA2\xE5\x64\xD4\xDB\xDD\x7F\xFB\x5F\x70\xD6\x8A\x0A\x64" 449063028418 
HSETNX _hash "\xCC\xDC\x41\xE4\x5B\x86\x63\x24\x47\x54\x17\x91\x4A\x6C\x0D\xD0\x48\xCC\x65\x3D\x0A\x20\x62\xEF\x50\xC0\x48\x8E\x52\x36\x8A\xC7\xF2\x5B\x21\x83\x5C\x58\x75\x6E\xB8\xB5\x2D\x14\xAA\x6E\xE2\x41\x2E\x58\x2C\x04\x06\x8E\xFA\x3B\x77\xE8\xAE\x1F\xF9\xFF\x1D\xA1\x09\xC1\x11\xD4\x4F\x04\x63\x45\xA8\xAE\x7D\xC6\xB8\x27\x17\x8C\xD4\xF8\xB8\x9E\x45\xEB\xA6\x89\xE5\x58\xD9\xA5\xE1\x6D\x5A\x62\x89\xFD\xDF\xE5\xA5\x17\x29\x20\x6C\xE7\xFF\xD0\x1E\x06\xF7\xB0\x99\x69\xA4\x82\x48\xB5\x38\x20\x7C\x7A\x7F\x68\xB5\x5B\xA8\x0B\x75\x5D\x38\x40\xA5\x74\x53\x95\x16\x21\x46\x62\x4D\x05\x51\x5A\x5B\x1E\xA1\x3D\xDD\x16\x47\x91\x0F\x10\x82\x3A\x6D\xAE\x2B\x83\xF7\x45\xDB\x09\xA8\x2B\x9F\xEB\x57\x96\x75\xE5\xC8\x76\x2E\xD4\x63\xCC\x77\xD2\x45\x3A\x36\xD9\x1B\x30\x05\xCE\xD2\xEF\xC7\xD1\xB3\x53\x19\xD7\xE0\xE3\x3E\x2E\x9B\xE0\xD5\x6B\x0A\x55\xF6\xC1\x75\x41\xB7\x49\x9D\xC9\xD4\xF0\x16\xA4\x93\xE0\xC9\x74\xAB\xE0\xD5\xE8\xB0\xF7\x34\xA5\xF2\x88\x1E\xD3\xB9\x1A\x3F\x7D\x46\x71\x04\x75\x3D" PtCj;Qy9lFaR?6BbndsWGRn2sc?vp?b[D`3Kuv3fA2EvOp<D70v<ayS>JW[QmLM6lm5>LD1hgnRUomIS2ctTf4<I@=9K8iVX>be`FITXqjA:4GMdU6<=PF@?KYw?WJb 
HGET _hash -348 
HINCRBYFLOAT _hash -996316403 -528 
HGETALL _hash 
HSET _hash -957798247671 687647969316 
HINCRBYFLOAT _hash 391404804024 -17[3892](https://github.com/enjoy-binbin/redis/actions/runs/5634670831/job/15264829706#step:6:3893)5306 
HSETNX _hash 200 239 
HVALS _hash 
HINCRBY _hash 743508623327 215058093525 
HMGET _hash 319 
HSET _hash -466 30442022220142111442144414111240202031013434000404403 
HKEYS _hash 
HVALS _hash 
HEXISTS _hash -153 
HMGET _hash "\xE6\x66\x76\x01\x9F\xFC\x27\xCE\xE2\x3C\x8C\x2F\x8E\x3A\xCD\x9F\x97\x72\x09\xFE\x07\x08\x06\xD1\x78\x04\xD6\x6A\x46\xC6\xB4\x18\xF9\x71\x31\xDD\xD6\x39\x7C\x88\xA2\x4F\x93\xAF\x04\x63\xF8\xEC\xD5\xDF\x96\x92\xFF\xF0\x71\xF4\xFC\xAA\x74\x2D\x26\x90\x8B\xAB\x6A\xC9\x97\x9F\xE3\x90\x7E\x4F\xDC\x07\xDF\x0F\x64\x62\x45\x91\x5A\x18\x7D\x76\x60\xEA\xFD\xA4\x06\x1C\x64\xF0\x47\x4D\x5E\x00\xAD\x28\xBB\xFC\xAE\x65\xBD\x22\x6A\x9F\x50\x46\xD3\x23\x10\x1D\xBA\x56\xC7\x3E\x97\x15\x85\x8E\x22\xC6\x9D\x97\xB7\x33\x24\x1A\x12\xD6\xD6\x8E\x56\xB9\x6F\x1E\x78\x5A\xBE\x3C\x3F\x80" 
HSTRLEN _hash 1487995316 
HSET _hash 831 -131 
HINCRBY _hash -42[3919](https://github.com/enjoy-binbin/redis/actions/runs/5634670831/job/15264829706#step:6:3920)645335 -349596092 
HSCAN _hash -741399613088 
HINCRBYFLOAT _hash 133070915 -890 
HSTRLEN _hash -955 
HMSET _hash -603 1997068011 
HSTRLEN _hash -113 
HVALS _hash 
HGET _hash "\x24\xE5\xBA\x1D\x22\x61\x2E\x93\x3B\x55\x12\xD5\x52\xC1\x3D\x59\x39\xFA\x3D\x78\xDF\xE4\xBF\x43\xBF\x56\xE4\x43\x18\x29\x6D\x2B\x3F\xE9\xDF\x60\x36\xE7\xC1\x0C\xE8\x55\xC4\x8F\x59\x32\x76\x60\xF6\x71\x0A\x86\x28\xD0\x4A\x18\x20\xCA\x89\x3C\x6C\x12\x5A\xD7\xC8\x1E\xD9\xA7\x8F\xD0\x67\xD9\x51\xB5\x3F\x0A\xB8\xAC\xB4\xD8\x1D\xEA\x25\x3E\xCF\xB2\xBA\x0B\xA2\x16\x29\x02\x4C\xE6\xEB\xE5\x17\x34\x8B\x63\x35\xD3\xAE\x26\x15\xBB\x0B\xE4\xCB\x06\x50\xDE\xA8\x5C\x16\xAA\xA8\x0B\xAE\x59\xE2\x7F\x50\x87\x89\x7B\xC8\xBF\xA8\x8D\xA9\xE3\x48\x66\xE0\x1A\x23\x89\xE6\xFB\x8D\x3D\xF1\x95\x7E\x78\xE8\x9F\x61\x65\xD3\xDF\x85\x7F\x48\x64\x45\x87\x10\xE9\x65\xAF\x12\xAE\xC1\x08\x0F\xD2\xFA\x10\x6F\x0F\x75\x95\x8A\x40\xFC\x07\xDA\xEC\x68\x38\x08\xC1\x25\xD1\xF1\xA8\xB9\x82\xFA\xBB\x86\xF5\xAE" 
HDEL _hash vBcf6iV5fhGf=neZ_P`v636TQ`xPUX]^OtXhAwWRMcfweP^IQhioRYgc[k=S5r[HOG=kJO9kCM3a9dP[NVpfsCG^>US5gj]ZyVkt;^@pr 
HMSET _hash -1832423111 -585378608 
HDEL _hash -852494368 
HSETNX _hash 17418093 984 
HVALS _hash 
HSTRLEN _hash -805533443487 
HDEL _hash 024231031044212302131042112 
HEXISTS _hash 3789913691 
HKEYS _hash 
HDEL _hash 410546534885 
HSCAN _hash "\x62\x1E\x57\x74\x1F\x18\x85\xBF\xA1\x3D\xF6\x24\x0E\xA9\xF7\xF7\x11\xB1\x2B\x4B\x85\x75\x24\x7E\x08\x53\x3D\x5D\x5E\xCE\xBF\x4A\xF6\xED\x60\xC4\xF6\x61\x77\xBC\x08\x2F\xB5\x56\x68\xF6\x6E\x1F\x40\x1B\x31\xFF\xDC\xDF\x0D\x7E\xD5\xEB\xBD\x53\xAE\xA7\x2F\x6E\xF4\x71\x24\x41\xC4\xC7\x11\x39\x10\x4B\xE7\x65\xB6\x68\xFF\xB0\x26\x28\x5F\x5D\xEE\x04\x9F\xC6\xFE\x64\x3D\x58\xF1\x74\x34\x82\x8F\x03\x33\x6C\x95\xE5\x20\xD0\x28\x78\xE2\x7F\xD2\x2C\xCB\x52\xAA\xE8\x19\xA8\x8A\x18\xD0\xF9\x32\xB0\x8C\x6A\x8C\xBE\x73\x14\x27\x70\x0A\x5F\xAD\x36\x31\x76\xE8\x3F\x1D\xE7\x09\xAE\xB2\xBD\xA4\x6C\x42\x1F\xC4\x6A\xBC\x59\xA8\x32\x24\x09\x6D\x88\x7B\xD0\xF0\x6F\x89\x59\xFB\x53\xA7\xB0\xEB\x8C\x16\x88\xA4\x83\xEC\x81\x3F\x56\xB0\xC7\x70\xBF\x7F\x84\xA2\xB9\x81\x4F\x67\xB4\x30"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants