-
Notifications
You must be signed in to change notification settings - Fork 955
Relaxed RDB check for foreign RDB formats #2543
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
Conversation
Signed-off-by: Viktor Söderqvist <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2543 +/- ##
=========================================
Coverage 72.21% 72.22%
=========================================
Files 126 127 +1
Lines 70618 70713 +95
=========================================
+ Hits 50997 51070 +73
- Misses 19621 19643 +22
🚀 New features to boost your workflow:
|
Signed-off-by: Viktor Söderqvist <[email protected]>
ranshid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - small comment about message
Signed-off-by: Viktor Söderqvist <[email protected]>
hpatro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. Will be good to have coverage via adding foreign RDB version/type not supported. Might need to start a Redis server and take some snapshots and copy it our test assets.
Signed-off-by: Viktor Söderqvist <[email protected]>
… rdb Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
12f6a96 to
e03279c
Compare
Signed-off-by: Viktor Söderqvist <[email protected]>
@hpatro I have added various RDB files and tests. I created an RDB 12 with the commit just before #665. We actually had RDB 12 in valkey unstable before that PR was merged. The other files RDB 75 and RDB 987 with and without unknonw types were created by editing the binary files. The unknown type is just a string where I changed the type byte. |
ranshid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
left some minor comments to consider
Signed-off-by: Viktor Söderqvist <[email protected]>
In valkey-io#1604, we attempt to read future Valkey RDB formats, but we rejected foreign RDB formats, because of the risk that the opcodes and types added by other projects collide with the new types and opcodes added in recent Valkey versions. This change accepts foreign RDB versions but limits the types and opcodes to the ones that we can understand, to prevent misinterpretation of types/opcodes which could lead to undefined behavior. If unsupported RDB types or opcodes are seen, we error out. Additional changes: * Improve error reporting when encountering unknown RDB types in relaxed version check mode. * Tests for loading various RDB files. * Improvement to valkey-check-rdb to accept future and foreign RDB versions, including tests. --------- Signed-off-by: Viktor Söderqvist <[email protected]>
In #1604, we attempt to read future Valkey RDB formats, but we rejected foreign RDB formats, because of the risk that the opcodes and types added by other projects collide with the new types and opcodes added in recent Valkey versions. This change accepts foreign RDB versions but limits the types and opcodes to the ones that we can understand, to prevent misinterpretation of types/opcodes which could lead to undefined behavior. If unsupported RDB types or opcodes are seen, we error out. Additional changes: * Improve error reporting when encountering unknown RDB types in relaxed version check mode. * Tests for loading various RDB files. * Improvement to valkey-check-rdb to accept future and foreign RDB versions, including tests. --------- Signed-off-by: Viktor Söderqvist <[email protected]>
In valkey-io#1604, we attempt to read future Valkey RDB formats, but we rejected foreign RDB formats, because of the risk that the opcodes and types added by other projects collide with the new types and opcodes added in recent Valkey versions. This change accepts foreign RDB versions but limits the types and opcodes to the ones that we can understand, to prevent misinterpretation of types/opcodes which could lead to undefined behavior. If unsupported RDB types or opcodes are seen, we error out. Additional changes: * Improve error reporting when encountering unknown RDB types in relaxed version check mode. * Tests for loading various RDB files. * Improvement to valkey-check-rdb to accept future and foreign RDB versions, including tests. --------- Signed-off-by: Viktor Söderqvist <[email protected]> Signed-off-by: Harkrishn Patro <[email protected]>
In #1604, we attempt to read future Valkey RDB formats, but we rejected foreign RDB formats, because of the risk that the opcodes and types added by other projects collide with the new types and opcodes added in recent Valkey versions.
This change accepts foreign RDB versions but limits the types and opcodes to the ones that we can understand, to prevent misinterpretation of types/opcodes which could lead to undefined behavior. If unsupported RDB types or opcodes are seen, we error out.
Additional changes: