Fix memory leak in RedisKey::hash_get_multi

[phartleyp]

This change fixes the memory leak in RedisKey::hash_get_multi()

It may not be a complete fix. There are several places where RedisString::new() is used but RedisString::from_redis_module_string() should be considered:

• RedisKeyWritable::list_pop_head() and RedisKeyWritable::list_pop_tail() should probably use RedisString::from_redis_module_string() by parallelism.
• ServerInfo also uses RedisString::new(). Needs review.

It may not be a reliable fix in all cases. redismodule-rs appears to permit Redis automatic memory management because there is a function Context::auto_memory() to enable automatic memory management. My change is untested with Redis automatic memory management. That said, I don't know if automatic memory management will play nice with any redismodule-rs features which depend on Rust RAII for memory management.

[oshadmi]

@phartleyp The fix for hash_mget_key looks good, thanks!
When RedisString is dropped, its inner RedisModuleString is freed and it is excluded from the auto memory, in case it is enabled - so it is OK whether auto memory is enabled or disabled.

We want to take the opportunity and add tests. Would you like to do that? (see tests/integration.rs)

The other calls to RediString::new, as you wrote, need further review - can be fixed in separate PRs.

[slavak]

It seems to me the root bug may be RedisString::new unnecessarily calling RedisModule_RetainString, which would increment the reference count to 2. Unless there's some scenario I'm missing where RedisString::new might be called with a string with refcount of 0.

In other words could it be that the implementation of RedisString::new should just be replaced with the body of RedisString::from_redis_module_string?

[oshadmi]

We might need both, so if we want to transfer ownership to the module, we use from_redis_module_string and when we are done with it, we decrement the ref count and possibly free it, and in addition, if we want redis to keep ownership, we use new which eventually does not decrement the initial ref count we started with.

[phartleyp]

In other words could it be that the implementation of RedisString::new should just be replaced with the body of RedisString::from_redis_module_string?

I did exactly that in my first attempt to fix this issue. The result was an instantaneous Redis crash as soon as I ran an integration test for my plugin module:

=== REDIS BUG REPORT START: Cut & paste starting from here ===
1:M 15 Jun 2022 03:38:14.494 # Redis 6.2.6 crashed by signal: 11, si_code: 1
1:M 15 Jun 2022 03:38:14.494 # Accessing address: 0xffffffffffffffff
1:M 15 Jun 2022 03:38:14.494 # Crashed running the instruction at: 0x5646d8ee4fe4