Skip to content

Commit

Permalink
Fix LPOS rank passing LLONG_MIN overflow issue (#1687)
Browse files Browse the repository at this point in the history
There is a minor overflow issue in RANK negation, passing LLONG_MIN
will overflow and is effectively be the same as passing -1.

This is the example before the fix:
```
127.0.0.1:6666> flushall
OK
127.0.0.1:6666> lpos mylist foo rank -9223372036854775808
(nil)
127.0.0.1:6666> lpush mylist foo foo foo foo foo
(integer) 5

127.0.0.1:6666> lpos mylist foo rank -1
(integer) 4
127.0.0.1:6666> lpos mylist foo rank -5
(integer) 0
127.0.0.1:6666> lpos mylist foo rank -6
(nil)
127.0.0.1:6666> lpos mylist foo rank -9223372036854775807
(nil)

-- this should return nil but it returned the last one because the overflow rank became -1
127.0.0.1:6666> lpos mylist foo rank -9223372036854775808
(integer) 4
```

Now we limit RANK to not be LLONG_MIN and will throw an error directly
(this is the behavior of Redis 7.2, but with different error words):
```
127.0.0.1:6666> lpos mylist foo rank -9223372036854775808
(error) ERR rank would overflow

127.0.0.1:6379> lpos mylist foo rank -9223372036854775808
(error) ERR value is out of range, value must between -9223372036854775807 and 9223372036854775807
```

Unrelated change: a small cleanup, return RedisParseErr instead of
RedisExecErr in Parse.
  • Loading branch information
enjoy-binbin authored Aug 21, 2023
1 parent 10861b0 commit 54423b6
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
7 changes: 5 additions & 2 deletions src/commands/cmd_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -697,16 +697,19 @@ class CommandLPos : public Commander {
"RANK can't be zero: use 1 to start from "
"the first match, 2 from the second ... "
"or use negative to start from the end of the list"};
} else if (spec_.rank == LLONG_MIN) {
// Negating LLONG_MIN will cause an overflow, and is effectively be the same as passing -1.
return {Status::RedisParseErr, "rank would overflow"};
}
} else if (parser.EatEqICase("count")) {
spec_.count = GET_OR_RET(parser.TakeInt());
if (spec_.count < 0) {
return {Status::RedisExecErr, "COUNT can't be negative"};
return {Status::RedisParseErr, "COUNT can't be negative"};
}
} else if (parser.EatEqICase("maxlen")) {
spec_.max_len = GET_OR_RET(parser.TakeInt());
if (spec_.max_len < 0) {
return {Status::RedisExecErr, "MAXLEN can't be negative"};
return {Status::RedisParseErr, "MAXLEN can't be negative"};
}
} else {
return {Status::RedisParseErr, errInvalidSyntax};
Expand Down
5 changes: 5 additions & 0 deletions tests/gocase/unit/type/list/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,11 @@ func TestList(t *testing.T) {
require.Equal(t, "bar", rdb.LRange(ctx, "target", 0, -1).Val()[0])
})

t.Run("LPOS rank negation overflow", func(t *testing.T) {
require.NoError(t, rdb.Del(ctx, "mylist").Err())
util.ErrorRegexp(t, rdb.Do(ctx, "LPOS", "mylist", "foo", "RANK", "-9223372036854775808").Err(), ".*rank would overflow.*")
})

for listType, large := range largeValue {
t.Run(fmt.Sprintf("LPOS basic usage - %s", listType), func(t *testing.T) {
createList("mylist", []string{"a", "b", "c", large, "2", "3", "c", "c"})
Expand Down

0 comments on commit 54423b6

Please sign in to comment.