test_helper example module crashes valkey-server

[dmitrypol]

REPRO STEPS
Run integration test_helper_version test
Notice how test_helper._version_rm_call fails sporadically

Run ./build.sh to compile all examples
Run valkey-server 7.2.5 and loadmodule .../target/debug/examples/libtest_helper.dylib
Run valkey-cli and execute test_helper._version_rm_call and test_helper.err

RESULT
Exception has occurred.

test_helper.version and test_helper.name work fine.

Same issue happens with redismodule-rs and redis-server.

[mkmkme]

I think this was fixed with valkey-io/valkey#608

Could you recheck it?

[mkmkme]

@dmitrypol so I was poking into this for a bit and I realized two things:

1. test_helper._version_rm_call does not have any issues by itself. You can run it as many times as you want and it always returns the same result without crashing the server. It causes the test failure because the version does not match. This is caused by the fact info server on Valkey 8.0.0 returns redis_version:7.2.4 and valkey_version:8.0.0 which makes ctx.get_redis_version_rm_call return 7.2.4
2. test_helper.err, however, has two major issues.

---

Issue 1: None unwrapping

test_helper_err has

    if args.len() < 1 {
        return Err(ValkeyError::WrongArity);
    }

    let msg = args.get(1).unwrap();

But args.len() includes the name of the function. Hence, it should check for args.len() < 2. And it should be called with an argument.

It's also supported by the first lines of valkey crash log:

thread '<unnamed>' panicked at examples/test_helper.rs:30:27:
called `Option::unwrap()` on a `None` value

I decided to just double-check that accessing the inaccessible argument would crash the server with this function:

fn test_helper_crash(ctx: &Context, args: Vec<ValkeyString>) -> ValkeyResult {
    println!("test_helper_crash called");
    if args.len() < 1 {
        return Err(ValkeyError::WrongArity);
    }

    let msg = args.get(1).unwrap();
    Ok(msg.into())
}

and indeed it crashes as well.

Issue 2: Double return

This was a tricky one. After fixing the args.len() issue I noticed that overall this function behaves inadequately. It would not register some function calls and then register a bunch. At the same time client would get the return for the earlier calls instead of a call they made.
So I decided to see what ctx.reply_error_string is doing and it seems to be a thin wrapper around ValkeyModule_ReplyWithError. However, after calling explicitly for ctx.reply_error_string the function has Ok(().into()) which calls ValkeyModule_ReplyWith* underneath for the second time in one function call.

I decided to see if I would reproduce the same confusion writing a function myself and I managed to do that with:

fn test_helper_err3(ctx: &Context, _args: Vec<ValkeyString>) -> ValkeyResult {
    println!("test_helper_err3 called");
    ctx.reply_error_string("test_helper_err3");
    Err(ValkeyError::Str("test_helper_err3 but return").into())
}

From valkey-cli behaviour and logs from the module I can see that the client-server communication gets very confused.

Since the module functions must return ValkeyResult I assume that the proper way to return an error would be to remove the explicit call for ctx.reply_error_string and replace Ok(().into()) with Err(ValkeyError::Str(msg.try_as_str().unwrap()))

---

All of this I covered in #99. Please review!