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 #14872: fix incorrectly handle with two-words redis command #14873

Merged
merged 5 commits into from Mar 20, 2020

Conversation

@mazhechao
Copy link
Contributor

mazhechao commented Dec 1, 2019

Fixes #14872

@mazhechao mazhechao requested a review from elastic/siem as a code owner Dec 1, 2019
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Dec 1, 2019

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Dec 1, 2019

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Member

adriansr left a comment

Thanks @mazhechao for reporting AND fixing this 😄

Can you add a entry to CHANGELOG.next.asciidoc too?

@@ -423,7 +424,7 @@ func (p *parser) parseArray(depth int, buf *streambuf.Buffer) (common.NetString,
}

// handle top-level request command
if depth == 0 && isRedisCommand(content[0]) {
if depth == 0 && (isRedisCommand(content[0]) || isRedisCommand(bytes.Join(content[0:2], []byte(" ")))) {

This comment has been minimized.

Copy link
@adriansr

adriansr Dec 2, 2019

Member

I think this needs a check in the (unlikely?) case that count < 2

This comment has been minimized.

Copy link
@mazhechao

mazhechao Dec 4, 2019

Author Contributor

Would you explain more details about what to check? I just don't understand what do you mean.

This comment has been minimized.

Copy link
@adriansr

adriansr Dec 4, 2019

Member

I mean accessing content[0:2] can fail if len(content) is 1, so you can write

 	if depth == 0 && (isRedisCommand(content[0]) || (count > 1 && isRedisCommand(bytes.Join(content[0:2], []byte(" "))))) { 

This comment has been minimized.

Copy link
@mazhechao

mazhechao Dec 4, 2019

Author Contributor

You are right. I get it.

@adriansr adriansr self-assigned this Dec 2, 2019
@mazhechao mazhechao force-pushed the mazhechao:master branch from 6c5e56c to 659fe8e Dec 4, 2019
@mazhechao

This comment has been minimized.

Copy link
Contributor Author

mazhechao commented Dec 4, 2019

@adriansr Would you please review again?

@@ -423,7 +424,7 @@ func (p *parser) parseArray(depth int, buf *streambuf.Buffer) (common.NetString,
}

// handle top-level request command
if depth == 0 && isRedisCommand(content[0]) {
if depth == 0 && (isRedisCommand(content[0]) || (count > 1 && isRedisCommand(bytes.Join(content[0:2], []byte(" "))))) {
p.message.isRequest = true
p.message.method = content[0]
if len(content) > 1 {

This comment has been minimized.

Copy link
@faec

faec Dec 5, 2019

Contributor

It seems like p.message.method and p.message.path need to be computed differently as well to handle the two-word case

This comment has been minimized.

Copy link
@mazhechao

mazhechao Dec 6, 2019

Author Contributor

I will have a look and test again.

@@ -70,6 +70,24 @@ func TestRedisParser_ArrayRequest(t *testing.T) {
assert.Equal(t, len(arrayRequest), msg.size)
}

var arrayRequest2 = []byte("*3\r\n" +
"$6\r\n" +

This comment has been minimized.

Copy link
@faec

faec Dec 5, 2019

Contributor

the extra whitespace here is probably what's breaking the linter... can you run mage fmt update and re-commit to make sure everything is formatted right?

This comment has been minimized.

Copy link
@mazhechao

mazhechao Dec 24, 2019

Author Contributor

I get errors, would you please help me?

\# command-line-arguments
./magefile.go:153:13: undefined: "github.com/elastic/beats/dev-tools/mage".DefaultIncludeListOptions
./magefile.go:156:39: not enough arguments in call to "github.com/elastic/beats/dev-tools/mage".GenerateIncludeListGo
Error: error compiling magefiles
@mazhechao mazhechao force-pushed the mazhechao:master branch from dd050f1 to c54e746 Dec 10, 2019
@mazhechao

This comment has been minimized.

Copy link
Contributor Author

mazhechao commented Dec 29, 2019

@adriansr @faec Would you please review my commits again?
And I don't know how to solve the lint problem, when I run mage fmt update, I get some errors

\# command-line-arguments
./magefile.go:153:13: undefined: "github.com/elastic/beats/dev-tools/mage".DefaultIncludeListOptions
./magefile.go:156:39: not enough arguments in call to "github.com/elastic/beats/dev-tools/mage".GenerateIncludeListGo
Error: error compiling magefiles 
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Feb 28, 2020

Pinging @elastic/siem (Team:SIEM)

@adriansr

This comment has been minimized.

Copy link
Member

adriansr commented Mar 20, 2020

jenkins, test this

@adriansr adriansr requested a review from faec Mar 20, 2020
@faec
faec approved these changes Mar 20, 2020
Copy link
Contributor

faec left a comment

Thank you!

@adriansr adriansr merged commit 30a5687 into elastic:master Mar 20, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA All commits passed the check
Details
adriansr added a commit to adriansr/beats that referenced this pull request Mar 20, 2020
elastic#14873)

Updated redis protocol decoder to support two-word commands.

Fixes elastic#14872

(cherry picked from commit 30a5687)
@adriansr adriansr added the v7.7.0 label Mar 20, 2020
adriansr added a commit that referenced this pull request Mar 20, 2020
…) (#17153)

Updated redis protocol decoder to support two-word commands.

Fixes #14872

(cherry picked from commit 30a5687)

Co-authored-by: shawshank <mazhechao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.