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

Fix GETEX not checking wrong type error causing key overwriting #1546

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

enjoy-binbin
Copy link
Member

This bug causes keys to be overwritten:

127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> type foo
list
127.0.0.1:6666> getex foo
""
127.0.0.1:6666> type foo
string

The reason is that we did not return early for wrong type
error and then the code overwrites it as a string key.
We should throw a wrong type error in this case:

127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> getex foo
(error) ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6666> type foo
list

GETEX was added in #961, look like this bug has existed since
the command was added.

This bug causes keys to be overwritten:
```
127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> type foo
list
127.0.0.1:6666> getex foo
""
127.0.0.1:6666> type foo
string
```

The reason is that we did not return early for wrong type
error and then the code overwrites it as a string key.
We should throw a wrong type error in this case:
```
127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> getex foo
(error) ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6666> type foo
list
```

GETEX was added in apache#961, look like this bug has existed since
the command was added.
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tisonkun tisonkun requested review from git-hulk and torwig July 5, 2023 11:46
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good catch

Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enjoy-binbin Good catch!

@xiaobiaozhao xiaobiaozhao merged commit 7e23c0b into apache:unstable Jul 5, 2023
@enjoy-binbin enjoy-binbin deleted the fix_getex_wrong_type branch July 5, 2023 14:59
@enjoy-binbin
Copy link
Member Author

enjoy-binbin commented Jul 6, 2023

Hi guys, i see we are doing squash merge but the commit message is empty.
Personally, I value the commit message. To be precise, i value the git blame log.
It is very helpful for us to track the PR changes, the change details etc.
My suggestion is to add appropriate message to express a certain commit when the code is merged.

In Redis community, we usually maintain the top comment of the PR. Both the proposer of the PR and the maintainer of the project have the right to modify the top comment (to reflect the latest status of PR). and when the time is come, maintainers can directly use top comment as commit message and do the squash merge. We can simply edit the top comment, copy those plain text and then do the squash merge

PS. it was discussed in #1553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants