Fix crash on dump command #440
Conversation
Handle non-UTF-8 binary data by using StringBuffer variant instead of panicking on unwrap. Co-authored-by: gkorland <[email protected]>
…ions Apply the same graceful error handling pattern to BigNumber and VerbatimString to prevent potential panics. Co-authored-by: gkorland <[email protected]>
| ctx.call("SET", &["test_dump_key", "test_value"])?; | ||
|
|
||
| // Call DUMP which returns binary data (may not be valid UTF-8) | ||
| let dump_result = ctx.call("DUMP", &["test_dump_key"])?; |
There was a problem hiding this comment.
Which cases does this cover? Seems valid UTF-8 String. What about the other cases, e.g., BigNumber, etc.
There was a problem hiding this comment.
DUMP is always on key name which is string.
There was a problem hiding this comment.
OK, although "test_value" is valid UTF-8, the dumped serialized value is handled as String and when containing invalid UTF-8 content it was failing unwrap and is now properly handled as StringBuffer.
The BigNumber and VerbatimString cases are just defensive against currently unreachable code (or need some mock to reach it)
| fn test_call_dump() -> Result<()> { | ||
| let mut con = TestConnection::new("call"); | ||
|
|
||
| let res: String = redis::cmd("call.dump_test") |
There was a problem hiding this comment.
maybe we should pass a string we want it to send to us back?
I just would like to get coverage on the RedisValue::StaticError case.
There was a problem hiding this comment.
I wouldn't abuse this test to test and cover the StaticError and other error
| RedisValue::StringBuffer(data) => { | ||
| if data.is_empty() { | ||
| return Err(RedisError::Str("DUMP returned empty data")); | ||
| } |
There was a problem hiding this comment.
Can also assert that data is invalid UTF-8 (otherwise should be a SimpleString), e.g.,
assert!(std::str::from_utf8(&data).is_err(), "DUMP should return invalid UTF-8 data");
3 participants
fix #439