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

test_helper example module crashes valkey-server #14

Open
dmitrypol opened this issue May 1, 2024 · 2 comments
Open

test_helper example module crashes valkey-server #14

dmitrypol opened this issue May 1, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@dmitrypol
Copy link
Collaborator

dmitrypol commented May 1, 2024

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.

@dmitrypol dmitrypol self-assigned this May 1, 2024
@dmitrypol dmitrypol added the bug Something isn't working label May 1, 2024
@mkmkme
Copy link
Contributor

mkmkme commented Jun 19, 2024

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

Could you recheck it?

@mkmkme
Copy link
Contributor

mkmkme commented Sep 24, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants