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

Index search with a bad external continuation value crashes API server #18

Open
mcesaro opened this issue Dec 19, 2024 · 2 comments
Open

Comments

@mcesaro
Copy link

mcesaro commented Dec 19, 2024

Passing an invalid continuation term to an index search crashes the API server.
The riak_index:decode_continuation/2 call will fail if the passed term can't be converted to binary:

%% @doc decode a continuation received from the outside world.
-spec decode_continuation(continuation() | undefined) -> last_result() | undefined.
decode_continuation(undefined) ->
    undefined;
decode_continuation(Bin) ->
    binary_to_term(base64:decode(Bin)).

If the passed term is invalid, the invocation will raise an exception:

* exception error: bad argument
     in function  binary_to_term/1
        called as binary_to_term(<<>>)
        *** argument 1: invalid external representation of a term
```

Trapping it inside a `try ... catch`  block to provide a sensible default would make  `apply_continuation/2` to gracefully fail.
@martinsumner
Copy link
Contributor

For the HTTP API it is better to validate inputs in the riak_kv_wm_index:malformed_request/2 callback, and then a correct 4xx response code will be returned if it is invalid.

So we probably should decode the continuation within the API, with a try .. catch clause as you suggest, and pass only correctly decoded continuations to the riak_index:to_index_query/2 function.

It would be good to then confirm this with an addition to the riak_test test for continuation.

@mcesaro
Copy link
Author

mcesaro commented Dec 21, 2024

Please note that I triggered the crash using the erlang grpc client and the continuation value I passed was an empty binary. So the error return value semantic might be bad_continuation_type and bad_continuation_value to handle both cases.

@tburghart tburghart transferred this issue from OpenRiak/riak_kv-forked Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants